diff mbox series

[PR86438] compare-elim: cope with set of in_b

Message ID orsh0b25rz.fsf@lxoliva.fsfla.org
State New
Headers show
Series [PR86438] compare-elim: cope with set of in_b | expand

Commit Message

Alexandre Oliva Nov. 8, 2018, 11 a.m. UTC 
When in_a resolves to a register set in the prev_clobber insn, we may
use the SET_SRC for the compare instead.  However, when in_b so
resolves, we proceed to use the reg with its earlier value.  When both
resolve to the same register and prev_clobber is an insn that modifies
the register, this arrangement may cause the compare to match (when it
shouldn't) and the elimination of the compare to incorrectly succeed.

(set (reg 1) (plus (reg 1) (const_int N)))
(set (reg 2) (reg 1))
(set (reg flags) (compare (reg 1) (reg 2)))

in_a: (reg 1)            --> (plus (reg 1) (const_int N))
in_b: (reg 2) -> (reg 1) -/> oops

(parallel [
 (set (reg flags) (compare (plus (reg 1) (const_int N))
                           (reg 1))) ;; should be (plus...)
 (set (reg 1) (plus (reg 1) (const_int N)))])
(set (reg 2) (reg 1))

This patch arranges for in_b to also undergo SET_SRC substitution
when appropriate, with a shortcut for when in_a and in_b are the same
rtx.

Bootstrapped, now regression-testing, on x86_64- and i686-linux-gnu.  Ok
to install if it passes?


for  gcc/ChangeLog

	PR rtl-optimization/86438
	* compare-elim.c (try_eliminate_compare): Use SET_SRC instead
	of in_b for the compare if in_b is SET_DEST.

for  gcc/testsuite/ChangeLog

	PR rtl-optimization/86438
	* gcc.dg/torture/pr86438.c: New.
---
 gcc/compare-elim.c                     |   25 +++++++++++++++++--------
 gcc/testsuite/gcc.dg/torture/pr86438.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr86438.c

Comments

Jeff Law Nov. 8, 2018, 9:57 p.m. UTC | #1 
On 11/8/18 4:00 AM, Alexandre Oliva wrote:
> When in_a resolves to a register set in the prev_clobber insn, we may
> use the SET_SRC for the compare instead.  However, when in_b so
> resolves, we proceed to use the reg with its earlier value.  When both
> resolve to the same register and prev_clobber is an insn that modifies
> the register, this arrangement may cause the compare to match (when it
> shouldn't) and the elimination of the compare to incorrectly succeed.
> 
> (set (reg 1) (plus (reg 1) (const_int N)))
> (set (reg 2) (reg 1))
> (set (reg flags) (compare (reg 1) (reg 2)))
> 
> in_a: (reg 1)            --> (plus (reg 1) (const_int N))
> in_b: (reg 2) -> (reg 1) -/> oops
> 
> (parallel [
>  (set (reg flags) (compare (plus (reg 1) (const_int N))
>                            (reg 1))) ;; should be (plus...)
>  (set (reg 1) (plus (reg 1) (const_int N)))])
> (set (reg 2) (reg 1))
> 
> This patch arranges for in_b to also undergo SET_SRC substitution
> when appropriate, with a shortcut for when in_a and in_b are the same
> rtx.
> 
> Bootstrapped, now regression-testing, on x86_64- and i686-linux-gnu.  Ok
> to install if it passes?
> 
> 
> for  gcc/ChangeLog
> 
> 	PR rtl-optimization/86438
> 	* compare-elim.c (try_eliminate_compare): Use SET_SRC instead
> 	of in_b for the compare if in_b is SET_DEST.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR rtl-optimization/86438
> 	* gcc.dg/torture/pr86438.c: New.
OK.
jeff
Alexandre Oliva Nov. 9, 2018, 10:09 a.m. UTC | #2 
On Nov  8, 2018, Jeff Law <law@redhat.com> wrote:

>> PR rtl-optimization/86438
>> * gcc.dg/torture/pr86438.c: New.
> OK.

Thanks.  I ended up tweaking the test a bit further, when it occurred to
me that I might be able to trigger the same problem with -m32, but no
such luck.  Here's the test I'm installing, instead of the one I'd
posted.

diff --git a/gcc/testsuite/gcc.dg/torture/pr86438.c b/gcc/testsuite/gcc.dg/torture/pr86438.c
new file mode 100644
index 000000000000..3e95515ae6a6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr86438.c
@@ -0,0 +1,29 @@
+/* { dg-do run } */
+
+typedef unsigned int u32;
+#if __SIZEOF_INT128__
+typedef unsigned long long u64;
+typedef unsigned __int128 u128;
+#else
+typedef unsigned long u64;
+typedef unsigned long long u128;
+#endif
+
+u128 g;
+
+static __attribute__ ((noinline, noclone))
+void check (u64 a, u64 b)
+{
+  if (a != 0 || b != 4)
+    __builtin_abort ();
+}
+
+int
+main (void)
+{
+  u64 d = (g ? 5 : 4);
+  u32 f = __builtin_sub_overflow_p (d, (u128) d, (u64) 0);
+  u128 x = g + f + d;
+  check (x >> (sizeof (u64) * __CHAR_BIT__), x);
+  return 0;
+}
diff mbox series

Patch

diff --git a/gcc/compare-elim.c b/gcc/compare-elim.c
index 50bbaa84b6df..8afbe76c502b 100644
--- a/gcc/compare-elim.c
+++ b/gcc/compare-elim.c
@@ -734,7 +734,7 @@  try_merge_compare (struct comparison *cmp)
 static bool
 try_eliminate_compare (struct comparison *cmp)
 {
-  rtx flags, in_a, in_b, cmp_src;
+  rtx flags, in_a, in_b, cmp_a, cmp_b;
 
   if (try_merge_compare (cmp))
     return true;
@@ -786,7 +786,7 @@  try_eliminate_compare (struct comparison *cmp)
 
   rtx x = XVECEXP (PATTERN (insn), 0, 0);
   if (rtx_equal_p (SET_DEST (x), in_a))
-    cmp_src = SET_SRC (x);
+    cmp_a = SET_SRC (x);
 
   /* Also check operations with implicit extensions, e.g.:
      [(set (reg:DI)
@@ -800,7 +800,7 @@  try_eliminate_compare (struct comparison *cmp)
 	   && (GET_CODE (SET_SRC (x)) == ZERO_EXTEND
 	       || GET_CODE (SET_SRC (x)) == SIGN_EXTEND)
 	   && GET_MODE (XEXP (SET_SRC (x), 0)) == GET_MODE (in_a))
-    cmp_src = XEXP (SET_SRC (x), 0);
+    cmp_a = XEXP (SET_SRC (x), 0);
 
   /* Also check fully redundant comparisons, e.g.:
      [(set (reg:SI)
@@ -811,7 +811,7 @@  try_eliminate_compare (struct comparison *cmp)
 	   && GET_CODE (SET_SRC (x)) == MINUS
 	   && rtx_equal_p (XEXP (SET_SRC (x), 0), in_a)
 	   && rtx_equal_p (XEXP (SET_SRC (x), 1), in_b))
-    cmp_src = in_a;
+    cmp_a = in_a;
 
   else
     return false;
@@ -819,17 +819,26 @@  try_eliminate_compare (struct comparison *cmp)
   /* If the source uses addressing modes with side effects, we can't
      do the merge because we'd end up with a PARALLEL that has two
      instances of that side effect in it.  */
-  if (side_effects_p (cmp_src))
+  if (side_effects_p (cmp_a))
+    return false;
+
+  if (in_a == in_b)
+    cmp_b = cmp_a;
+  else if (rtx_equal_p (SET_DEST (x), in_b))
+    cmp_b = SET_SRC (x);
+  else
+    cmp_b = in_b;
+  if (side_effects_p (cmp_b))
     return false;
 
   /* Determine if we ought to use a different CC_MODE here.  */
-  flags = maybe_select_cc_mode (cmp, cmp_src, in_b);
+  flags = maybe_select_cc_mode (cmp, cmp_a, cmp_b);
   if (flags == NULL)
     flags = gen_rtx_REG (cmp->orig_mode, targetm.flags_regnum);
 
   /* Generate a new comparison for installation in the setter.  */
-  rtx y = copy_rtx (cmp_src);
-  y = gen_rtx_COMPARE (GET_MODE (flags), y, in_b);
+  rtx y = copy_rtx (cmp_a);
+  y = gen_rtx_COMPARE (GET_MODE (flags), y, copy_rtx (cmp_b));
   y = gen_rtx_SET (flags, y);
 
   /* Canonicalize instruction to:
diff --git a/gcc/testsuite/gcc.dg/torture/pr86438.c b/gcc/testsuite/gcc.dg/torture/pr86438.c
new file mode 100644
index 000000000000..d4a23f313a55
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr86438.c
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+
+typedef unsigned int u32;
+typedef unsigned long long u64;
+#if __SIZEOF_INT128__
+typedef unsigned __int128 u128;
+#else
+typedef u64 u128;
+#endif
+
+u128 g;
+
+static __attribute__ ((noinline, noclone))
+void check (u64 a, u64 b)
+{
+  if (a != 0 || b != 4)
+    __builtin_abort ();
+}
+
+int
+main (void)
+{
+  u64 d = (g ? 5 : 4);
+  u32 f = __builtin_sub_overflow_p (d, (u128) d, (u64) 0);
+  u128 x = g + f + d;
+  check ((x >> 32) >> 32, x);
+  return 0;
+}