Re: [PATCH] fold-const: Don't access bit fields with too big mode (PR71310)

[Richard Biener <rguenther at suse dot de>]

On Fri, 10 Jun 2016, Segher Boessenkool wrote:

> Currently, optimize_bit_field_compare reads the bitfield in word_mode
> if it can.  If the bit field is normally accessed in a smaller mode,
> this might be a violation of the memory model, although the "extra"
> part of the read is not used.  But also, previous stores to the bit
> field will have been done in the smaller mode, and then bigger loads
> from it cause a LHS problem.
> 
> Bootstrapped and regchecked on powerpc64-linux.  Is this okay for
> trunk?

I think you miss a && DECL_BIT_FIELD_TYPE (TREE_OPERAND (lhs, 1))
after the COMPONENT_REF check.  Also while this change is certainly
correct it might be not complete dependent on how the code computes
the access offset - for the C++ memory model the access needs to
be constrained to the DECL_BIT_FIELD_REPRESENTATIVE extent.  That is,
a C++ memory model fix would be to supply the bitregion_start and
bitregion_end arguments to get_best_mode.  RTL expansion uses
expr.c:get_bit_range as a helper for this.  I guess the best thing
would be to export it and re-use it here.

Disclaimer: I think this "optimization" does not belong in
fold-const.c and is very premature at the time we invoke it
(when folding GENERIC from the frontends).

Thanks,
Richard.


> 
> Segher
> 
> 
> 2016-06-10  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR middle-end/71310
> 	* fold-const.c (optimize_bit_field_compare): Don't try to use
> 	word_mode unconditionally for reading the bit field, look at
> 	DECL_BIT_FIELD_REPRESENTATIVE instead.
> 
> ---
>  gcc/fold-const.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index 5058746..f067001 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -3903,13 +3903,24 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
>         return 0;
>     }
>  
> +  /* Don't use a larger mode for reading the bit field than we will
> +     use in other places accessing the bit field.  */
> +  machine_mode largest_mode = word_mode;
> +  if (TREE_CODE (lhs) == COMPONENT_REF)
> +    {
> +      tree field = TREE_OPERAND (lhs, 1);
> +      tree repr = DECL_BIT_FIELD_REPRESENTATIVE (field);
> +      if (repr)
> +	largest_mode = DECL_MODE (repr);
> +    }
> +
>    /* See if we can find a mode to refer to this field.  We should be able to,
>       but fail if we can't.  */
>    nmode = get_best_mode (lbitsize, lbitpos, 0, 0,
>  			 const_p ? TYPE_ALIGN (TREE_TYPE (linner))
>  			 : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
>  				TYPE_ALIGN (TREE_TYPE (rinner))),
> -			 word_mode, false);
> +			 largest_mode, false);
>    if (nmode == VOIDmode)
>      return 0;
>  
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)