| Message ID | 4c06e27d44ca971b0bc40f95e3bf6762147434b0.1465583814.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 06/10/2016 12:48 PM, 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? > > > 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. Testcase? It would seem to me that you could make a ppc testcase without major difficulties from either of the tests in the BZ. The change itself is fine, and it's approved with a testcase or at least an explanation of why you can't turn either of the tests from the BZ into a testcase in our framework. I do note there's two more calls to get_best_mode that unconditionally uses word_mode in fold_truth_andor_1. I think in that case we're looking to try and loads of adjacent fields that are being compared. I haven't thought much about whether or not it's safe WRT the C++11 memory model. Jeff
On Fri, Jun 10, 2016 at 01:14:16PM -0600, Jeff Law wrote: > > 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. > Testcase? It would seem to me that you could make a ppc testcase > without major difficulties from either of the tests in the BZ. Yeah I can do that, will have to deal with all our various ABIs, sigh ;-) Segher
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; > >
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;