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

[Richard Biener <rguenther@suse.de>]

On Thu, 11 Aug 2016, Bernd Edlinger wrote:

> On 08/11/16, Richard Biener wrote:
> > 
> > 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.
> > 
> 
> Oh yes, thanks for catching that!
> 
> If that information is true, that ought to go into the comment before
> the if, that would certainly be an interesting comment :-)
> 
> Are there any test cases for this non-constant field offsets?
> 
> I see many checks if TREE_TYPE of
> component_ref_field_offset is INTEGER_CST, but with very little
> background why it could be otherwise.
> 
> I think we should simply fall back to the BIT_FIELD_REF in that case,
> that would mean, the if should be something like:
> 
> tree offset = component_ref_field_offset (ref);
> if (boff % BITS_PER_UNIT != 0
>     || !tree_fits_uhwi_p (offset))
> 
> And yes, the correctness issue can certainly be solved with the
> BIT_FIELD_REF alone.
> 
> So, as requested, here is a first iteration of my patch that always builds
> a BIT_FIELD_REF, together with the test cases.
> 
> 
> Boot-strap & regression testing was done on x86_64-pc-linux-gnu.
> Is it OK for trunk (and active branches)?

Yes.

Thanks,
Richard.