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

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

[Segher Boessenkool]

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.

---
 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;
 
-- 
1.9.3

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

[Jeff Law]

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

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

[Segher Boessenkool]

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

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

[Segher Boessenkool]

On Fri, Jun 10, 2016 at 01:14:16PM -0600, Jeff Law wrote:
> 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.

This is what I committed:


2016-06-11  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/testsuite/
	PR middle-end/71310
	* gcc.target/powerpc/pr71310.c: New testcase.

---
 gcc/fold-const.c                           | 13 ++++++++++++-
 gcc/testsuite/gcc.target/powerpc/pr71310.c | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr71310.c

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/testsuite/gcc.target/powerpc/pr71310.c b/gcc/testsuite/gcc.target/powerpc/pr71310.c
new file mode 100644
index 0000000..6869a50
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr71310.c
@@ -0,0 +1,23 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-options "-O2" } */
+
+/* { dg-final { scan-assembler-not {\mld} } } */
+/* { dg-final { scan-assembler-not {\mlwz} } } */
+/* { dg-final { scan-assembler-times {\mlbz} 2 } } */
+
+struct mmu_gather {
+        long end;
+        int fullmm : 1;
+};
+
+void __tlb_reset_range(struct mmu_gather *p1)
+{
+        if (p1->fullmm)
+                p1->end = 0;
+}
+
+void tlb_gather_mmu(struct mmu_gather *p1)
+{
+        p1->fullmm = 1;
+        __tlb_reset_range(p1);
+}
-- 
1.9.3

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

[H.J. Lu]

On Fri, Jun 10, 2016 at 5:12 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Jun 10, 2016 at 01:14:16PM -0600, Jeff Law wrote:
>> 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.
>
> This is what I committed:
>
>
> 2016-06-11  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/testsuite/
>         PR middle-end/71310
>         * gcc.target/powerpc/pr71310.c: New testcase.
>
>

It caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71510


-- 
H.J.

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

[Richard Biener]

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)