diff mbox series

recog: Avoid validate_change shortcut for groups [PR115782]

Message ID mptmsmp4yl7.fsf@arm.com
State Committed
Commit 44fc801e97a8dc626a4806ff4124439003420b20
Headers
Series recog: Avoid validate_change shortcut for groups [PR115782] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Richard Sandiford July 10, 2024, 5:15 a.m. UTC 
  In this PR, due to the -f flags, we ended up with:

bb1:  r10=r10
...
bb2:  r10=r10
...
bb3:  ...=r10

with bb1->bb2 and bb1->bb3.

late-combine successfully combined the bb1->bb2 def-use and set
the insn code to NOOP_MOVE_INSN_CODE.  The bb1->bb3 combination
then failed for... reasons.  At this point, everything should have
been rewound to its original state.

However, substituting r10=r10 into r10=r10 gives r10=r10, and
validate_change had an early-out for no-op rtl changes.  This meant
that validate_change did not register a change for the bb2 insn and
so did not save its old insn code.  The NOOP_MOVE_INSN_CODE therefore
persisted even after the attempt had been rewound.

IMO it'd be too cumbersome and error-prone to expect all users of
validate_change to be aware of this possibility.  If code is using
validate_change with in_group=1, I think it has a reasonable expectation
that a change will be registered and that the insn code will be saved
(and restored on cancel).  This patch therefore limits the shortcut
to the !in_group case.

Bootstrapped & regression-tested on aarch64-linux-gnu & x86_64-linux-gnu.
OK to install?

Richard


gcc/
	PR rtl-optimization/115782
	* recog.cc (validate_change_1): Suppress early exit for no-op
	changes that are part of a group.

gcc/testsuite/
	PR rtl-optimization/115782
	* gcc.dg/pr115782.c: New test.
---
 gcc/recog.cc                    |  7 ++++++-
 gcc/testsuite/gcc.dg/pr115782.c | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr115782.c

Comments

Jeff Law July 10, 2024, 10:01 p.m. UTC | #1 
On 7/9/24 11:15 PM, Richard Sandiford wrote:
> In this PR, due to the -f flags, we ended up with:
> 
> bb1:  r10=r10
> ...
> bb2:  r10=r10
> ...
> bb3:  ...=r10
> 
> with bb1->bb2 and bb1->bb3.
> 
> late-combine successfully combined the bb1->bb2 def-use and set
> the insn code to NOOP_MOVE_INSN_CODE.  The bb1->bb3 combination
> then failed for... reasons.  At this point, everything should have
> been rewound to its original state.
> 
> However, substituting r10=r10 into r10=r10 gives r10=r10, and
> validate_change had an early-out for no-op rtl changes.  This meant
> that validate_change did not register a change for the bb2 insn and
> so did not save its old insn code.  The NOOP_MOVE_INSN_CODE therefore
> persisted even after the attempt had been rewound.
> 
> IMO it'd be too cumbersome and error-prone to expect all users of
> validate_change to be aware of this possibility.  If code is using
> validate_change with in_group=1, I think it has a reasonable expectation
> that a change will be registered and that the insn code will be saved
> (and restored on cancel).  This patch therefore limits the shortcut
> to the !in_group case.
> 
> Bootstrapped & regression-tested on aarch64-linux-gnu & x86_64-linux-gnu.
> OK to install?
> 
> Richard
> 
> 
> gcc/
> 	PR rtl-optimization/115782
> 	* recog.cc (validate_change_1): Suppress early exit for no-op
> 	changes that are part of a group.
> 
> gcc/testsuite/
> 	PR rtl-optimization/115782
> 	* gcc.dg/pr115782.c: New test.
OK
jeff
diff mbox series

Patch

diff --git a/gcc/recog.cc b/gcc/recog.cc
index 36507f3f57c..7710c55b745 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -230,7 +230,12 @@  validate_change_1 (rtx object, rtx *loc, rtx new_rtx, bool in_group,
       new_len = -1;
     }
 
-  if ((old == new_rtx || rtx_equal_p (old, new_rtx))
+  /* When a change is part of a group, callers expect to be able to change
+     INSN_CODE after making the change and have the code reset to its old
+     value by a later cancel_changes.  We therefore need to register group
+     changes even if they're no-ops.  */
+  if (!in_group
+      && (old == new_rtx || rtx_equal_p (old, new_rtx))
       && (new_len < 0 || XVECLEN (new_rtx, 0) == new_len))
     return true;
 
diff --git a/gcc/testsuite/gcc.dg/pr115782.c b/gcc/testsuite/gcc.dg/pr115782.c
new file mode 100644
index 00000000000..f4d11cc6d0f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr115782.c
@@ -0,0 +1,23 @@ 
+// { dg-require-effective-target lp64 }
+// { dg-options "-O2 -fno-guess-branch-probability -fgcse-sm -fno-expensive-optimizations -fno-gcse" }
+
+int printf(const char *, ...);
+int a, b, c, d, e, f, g, i, j, m, h;
+long k, l, n, o;
+int main() {
+  int p = e, r = i << a, q = r & b;
+  k = 4073709551613;
+  l = m = c = -(c >> j);
+  d = g ^ h ^ 4073709551613;
+  n = q - h;
+  o = ~d;
+  f = c * 4073709551613 / 409725 ^ r;
+  if ((n && m) || (q && j) || a)
+    return 0;
+  d = o | p;
+  if (g)
+    printf("0");
+  d = p;
+  c++;
+  return 0;
+}