Re: [PATCH] Fix unaligned access when predictive commoning (PR 71083)

[Richard Biener <rguenther@suse.de>]

On Wed, 10 Aug 2016, Bernd Edlinger wrote:

> On 08/10/16 14:29, Richard Biener wrote:
> > On Tue, 9 Aug 2016, Bernd Edlinger wrote:
> >> We know that the difference between the innermost ref
> >> and the outer ref is byte-aligned, but we do not know
> >> that the same is true for offset between the COMPONENT_REF
> >> and the outer ref.
> >>
> >> I mean boff is essentially the difference between
> >> bitpos of get_inner_reference(exp) and
> >> bitpos of get_inner_reference(TREE_OPERAND (exp, 0))
> >>
> >> This would be exposed by my patch, because previously
> >> we always generated BIT_FIELD_REFS, with bit-offset 0,
> >> but the MEM_REF at the byte-offset there is in all likelihood
> >> pretty much unaligned, the MEM_REF at the COMPONENT_REF
> >> is likely more aligned and allows better code for ARM processors,
> >> but only if the MEM_REF is at a byte-aligned offset at all,
> >> otherwise the whole transformation would be wrong.
> >
> > Note that the important thing to ensure is that the access the
> > MEM_REF performs is correct TBAA-wise which means you either
> > have to use alias-set zero (ptr_type_node offset) or _not_
> > shuffle the offset arbitrarily between the MEM_REF and the
> > components you wrap it in.
> >
> > Richard.
> >
> 
> Yes, the patch exactly replicates the outermost COMPONENT_REF and
> subtracts the component's byte-offset from the MEM_REF's address,
> and the MEM_REF uses the pointer type of the inner reference.
> 
> In the case without bitfields and the Ada bitfields the patch changes
> nothing, except we build an aligned type out of TREE_TYPE (DR_REF (dr))
> and get_object_alignment (DR_REF (dr)).
> 
> In the case with a component_ref that is byte-aligned
> we subtract the component byte offset from the address
> before the MEM_REF is constructed.  And the
> alias_ptr is of type reference_alias_ptr_type
> (TREE_OPERAND (DR_REF (dr), 0)) and again the alignment
> from get_object_alignment (TREE_OPERAND (DR_REF (dr), 0)
> so that should be exactly type-correct from TBAA's perspective.
> 
> 
> Attached a new version of the patch with an improved comment,
> and the new Ada test cases.
> 
> 
> Bootstrap and reg-test on x86_64-pc-linux-gnu without regression.
> Is it OK for trunk?

The patch looks mostly ok, but

+      else
+       {
+         boff >>= LOG2_BITS_PER_UNIT;
+         boff += tree_to_uhwi (component_ref_field_offset (ref));
+         coff = size_binop (MINUS_EXPR, coff, ssize_int (boff));

how can we be sure that component_ref_field_offset is an INTEGER_CST?
At least Ada can have variably-length fields before a bitfield.  We'd
need to apply component_ref_field_offset to off in that case.  Which
makes me wonder if we can simply avoid the COMPONENT_REF path in
a first iteration of the patch and always build a BIT_FIELD_REF?
It should solve the correctness issues as well and be more applicable
for branches.

Thanks,
Richard.