[PATCH] Fix PR rtl-optimization/67736 in combine.c

[PATCH] Fix PR rtl-optimization/67736 in combine.c

[Steve Ellcey]

This is a new patch to fix PR rtl-optimization/67736.  My original patch
was MIPS specific and that was rejected and Andrew Pinski pointed me to
a patch he had sent out several years ago but that was never checked in.
I updated the patch and tested it with no regressions and would like to
check it in.  I addressed Eric Botcazou comments from the original patch
and also included the (updated) test case that Andrew had as well as one
I created.

OK to checkin?

Steve Ellcey
sellcey@imgtec.com

My original patch and the follow up discussion is at:

https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02111.html

Andrews original patch is at:

https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00401.html



2015-10-21  Steve Ellcey  <sellcey@imgtec.com>
	    Andrew Pinski  <apinski@cavium.com>

	PR rtl-optimization/67736
	* combine.c (simplify_comparison): Use gen_lowpart_or_truncate instead
	of gen_lowpart when we had a truncating and.


diff --git a/gcc/combine.c b/gcc/combine.c
index fd3e19c..7568ebb 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -11529,8 +11529,8 @@ simplify_comparison (enum rtx_code code, rtx *pop0, rtx *pop1)
 		 tmode != GET_MODE (op0); tmode = GET_MODE_WIDER_MODE (tmode))
 	      if ((unsigned HOST_WIDE_INT) c0 == GET_MODE_MASK (tmode))
 		{
-		  op0 = gen_lowpart (tmode, inner_op0);
-		  op1 = gen_lowpart (tmode, inner_op1);
+		  op0 = gen_lowpart_or_truncate (tmode, inner_op0);
+		  op1 = gen_lowpart_or_truncate (tmode, inner_op1);
 		  code = unsigned_condition (code);
 		  changed = 1;
 		  break;
@@ -12048,12 +12048,9 @@ simplify_comparison (enum rtx_code code, rtx *pop0, rtx *pop1)
 				   & GET_MODE_MASK (mode))
 				  + 1)) >= 0
 	      && const_op >> i == 0
-	      && (tmode = mode_for_size (i, MODE_INT, 1)) != BLKmode
-	      && (TRULY_NOOP_TRUNCATION_MODES_P (tmode, GET_MODE (op0))
-		  || (REG_P (XEXP (op0, 0))
-		      && reg_truncated_to_mode (tmode, XEXP (op0, 0)))))
+	      && (tmode = mode_for_size (i, MODE_INT, 1)) != BLKmode)
 	    {
-	      op0 = gen_lowpart (tmode, XEXP (op0, 0));
+	      op0 = gen_lowpart_or_truncate (tmode, XEXP (op0, 0));
 	      continue;
 	    }
 


2015-10-21  Steve Ellcey  <sellcey@imgtec.com>
	    Andrew Pinski  <apinski@cavium.com>

	PR rtl-optimization/67736
	* gcc.dg/torture/pr67736.c: New test.
	* gcc.dg/combine-subregs.c: New test.


diff --git a/gcc/testsuite/gcc.dg/combine-subregs.c b/gcc/testsuite/gcc.dg/combine-subregs.c
index e69de29..c480f88 100644
--- a/gcc/testsuite/gcc.dg/combine-subregs.c
+++ b/gcc/testsuite/gcc.dg/combine-subregs.c
@@ -0,0 +1,36 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fexpensive-optimizations" } */
+
+#include <inttypes.h>
+#include <stdlib.h>
+
+void __attribute__ ((noinline))
+foo (uint64_t state, uint32_t last)
+{
+  if (state == last) abort ();
+}
+
+/* This function may do a bad comparision by trying to
+   use SUBREGS during the compare on machines where comparing
+   two registers always compares the entire register regardless
+   of mode.  */
+
+int __attribute__ ((noinline))
+compare (uint64_t state, uint32_t *last, uint8_t buf)
+{
+    if (*last == ((state | buf) & 0xFFFFFFFF)) {
+	foo (state, *last);
+        return 0;
+    }
+    return 1;
+}
+
+int
+main(int argc, char **argv) {
+    uint64_t state = 0xF00000100U;
+    uint32_t last  = 0x101U;
+    int ret        = compare(state, &last, 0x01);
+    if (ret != 0)
+	abort ();
+    exit (0);
+}
diff --git a/gcc/testsuite/gcc.dg/torture/pr67736.c b/gcc/testsuite/gcc.dg/torture/pr67736.c
index e69de29..b45874e 100644
--- a/gcc/testsuite/gcc.dg/torture/pr67736.c
+++ b/gcc/testsuite/gcc.dg/torture/pr67736.c
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+typedef unsigned long long uint64_t;
+void f(uint64_t *a, uint64_t aa) __attribute__((noinline));
+void f(uint64_t *a, uint64_t aa)
+{
+  uint64_t new_value = aa;
+  uint64_t old_value = *a;
+  int bit_size = 32;
+    uint64_t mask = (uint64_t)(unsigned)(-1);
+    uint64_t tmp = old_value & mask;
+    new_value &= mask;
+    /* On overflow we need to add 1 in the upper bits */
+    if (tmp > new_value)
+        new_value += 1ull<<bit_size;
+    /* Add in the upper bits from the old value */
+    new_value += old_value & ~mask;
+    *a = new_value;
+}
+int main(void)
+{
+  uint64_t value, new_value, old_value;
+  value = 0x100000001;
+  old_value = value;
+  new_value = (value+1)&(uint64_t)(unsigned)(-1);
+  f(&value, new_value);
+  if (value != old_value+1)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] Fix PR rtl-optimization/67736 in combine.c

[Segher Boessenkool]

On Thu, Oct 22, 2015 at 09:47:23AM -0700, Steve Ellcey  wrote:
> OK to checkin?

> 2015-10-21  Steve Ellcey  <sellcey@imgtec.com>
> 	    Andrew Pinski  <apinski@cavium.com>
> 
> 	PR rtl-optimization/67736
> 	* combine.c (simplify_comparison): Use gen_lowpart_or_truncate instead
> 	of gen_lowpart when we had a truncating and.

This is okay for mainline, and the release branches after the usual delay
and watching for fallout.

> 2015-10-21  Steve Ellcey  <sellcey@imgtec.com>
> 	    Andrew Pinski  <apinski@cavium.com>
> 
> 	PR rtl-optimization/67736
> 	* gcc.dg/torture/pr67736.c: New test.
> 	* gcc.dg/combine-subregs.c: New test.

One nit and maybe a problem:

> --- a/gcc/testsuite/gcc.dg/combine-subregs.c
> +++ b/gcc/testsuite/gcc.dg/combine-subregs.c
> @@ -0,0 +1,36 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fexpensive-optimizations" } */

-fexpensive-optimizations is default at -O2.

> +
> +#include <inttypes.h>

Does every target have that header?  Shouldn't it be <stdint.h>?

> --- a/gcc/testsuite/gcc.dg/torture/pr67736.c
> +++ b/gcc/testsuite/gcc.dg/torture/pr67736.c
> @@ -0,0 +1,32 @@
> +/* { dg-do run } */
> +
> +#include <stdlib.h>
> +

And here you don't need inttypes at all?  Confused.


Segher

Re: [PATCH] Fix PR rtl-optimization/67736 in combine.c

[Steve Ellcey]

On Thu, 2015-10-22 at 13:46 -0500, Segher Boessenkool wrote:

> One nit and maybe a problem:
> 
> > --- a/gcc/testsuite/gcc.dg/combine-subregs.c
> > +++ b/gcc/testsuite/gcc.dg/combine-subregs.c
> > @@ -0,0 +1,36 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -fexpensive-optimizations" } */
> 
> -fexpensive-optimizations is default at -O2.

You are right, the original report I got mentioned this flag and then I
was comparing -fexpensive-optimization and -fno-expensive-optimization
so I didn't notice that it was on by default.  I will remove it from the
test.

> > +
> > +#include <inttypes.h>
> 
> Does every target have that header?  Shouldn't it be <stdint.h>?
> 
> > --- a/gcc/testsuite/gcc.dg/torture/pr67736.c
> > +++ b/gcc/testsuite/gcc.dg/torture/pr67736.c
> > @@ -0,0 +1,32 @@
> > +/* { dg-do run } */
> > +
> > +#include <stdlib.h>
> > +
> 
> And here you don't need inttypes at all?  Confused.
> 
> 
> Segher

Not including the include in pr67736.c was an oversight, I meant to
include it.  I didn't notice that Andrew had put in his own definition
of uint64_t into this test so it didn't need any includes, that is why
it still worked.  But you are right I should be using stdint.h instead
of inttypes.h.  It looks like most tests have:

/* { dg-do run { target { stdint_types } } } */
#include <stdint.h>

So I will use that in both tests and remove the local definition of
uint64_t from pr67736.c.

Steve Ellcey
sellcey@imgtec.com