[COMMITTED] testsuite/gcc.target/cris/pr93372-2.c: Handle xpass from combine improvement

[COMMITTED] testsuite/gcc.target/cris/pr93372-2.c: Handle xpass from combine improvement

[Hans-Peter Nilsson]

The xpassing change in generated code was as follows, at
r14-9788-gb7bd2ec73d66f7 (where I locally applied a revert
to verify that this suspect was the cause).  That was so
much of an improvement that I had to share it!  Worth the
testsuite churn anyway. :)

Segher, if you end up reverting r14-9692-g839bc42772ba7a (as
unfortunately seems not unlikely), then please also revert this
commit: r14-9799-g4c8b3600c4856f7915281ae3ff4d97271c83a540.

--- pr93372-2.s.pre	2024-04-05 01:49:47.985685902 +0200
+++ pr93372-2.s.post	2024-04-05 01:42:02.296489730 +0200
@@ -5,12 +5,9 @@
 	.global _f
 	.type	_f, @function
 _f:
-	move.d $r10,$r9
-	sub.d $r11,$r9
-	cmp.d $r11,$r10
-	seq $r10
-	move.d $r10,[$r12]
-	cmpq 0,$r9
+	sub.d $r11,$r10
+	seq $r9
+	move.d $r9,[$r12]
 	ret
 	sge $r10
 

-- >8 --
After r14-9692-g839bc42772ba7a, a sequence that actually
looks optimal is now emitted, observed at
r14-9788-gb7bd2ec73d66f7.  This caused an XPASS for this
test.  While adjusting the test, better also guard it
against regressions by checking that there are no redundant
move insns.

That's the only test that's improved to the point of
affecting test-patterns.  E.g. pr93372-5.c (which references
pr93372-2.c) is also improved, though it retains a redundant
compare insn.  (PR 93372 was about regressions from the cc0
representation; not further improvement like here, thus it's
not tagged.  Though, I did not double-check whether this
actually *was* a regression from cc0.)

	* gcc.target/cris/pr93372-2.c: Tweak scan-assembler
	checks to cover recent combine improvement.
---
 gcc/testsuite/gcc.target/cris/pr93372-2.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/gcc/testsuite/gcc.target/cris/pr93372-2.c b/gcc/testsuite/gcc.target/cris/pr93372-2.c
index 912069c018d5..2ef6471a990b 100644
--- a/gcc/testsuite/gcc.target/cris/pr93372-2.c
+++ b/gcc/testsuite/gcc.target/cris/pr93372-2.c
@@ -1,19 +1,20 @@
 /* Check that eliminable compare-instructions are eliminated. */
 /* { dg-do compile } */
 /* { dg-options "-O2" } */
-/* { dg-final { scan-assembler-not "\tcmp|\ttest" { xfail *-*-* } } } */
-/* { dg-final { scan-assembler-not "\tnot" { xfail cc0 } } } */
-/* { dg-final { scan-assembler-not "\tlsr" { xfail cc0 } } } */
+/* { dg-final { scan-assembler-not "\tcmp|\ttest" } } */
+/* { dg-final { scan-assembler-not "\tnot" } } */
+/* { dg-final { scan-assembler-not "\tlsr" } } */
+/* We should get just one move, storing the result into *d.  */
+/* { dg-final { scan-assembler-times "\tmove" 1 } } */
 
 int f(int a, int b, int *d)
 {
   int c = a - b;
 
-  /* Whoops!  We get a cmp.d with the original operands here. */
+  /* We used to get a cmp.d with the original operands here. */
   *d = (c == 0);
 
-  /* Whoops!  While we don't get a test.d for the result here for cc0,
-     we get a sequence of insns: a move, a "not" and a shift of the
-     subtraction-result, where a simple "spl" would have done. */
+  /* We used to get a suboptimal sequence, but now we get the optimal "sge"
+     (a.k.a "spl") re-using flags from the subtraction. */
   return c >= 0;
 }
-- 
2.30.2

Re: [COMMITTED] testsuite/gcc.target/cris/pr93372-2.c: Handle xpass from combine improvement

[Segher Boessenkool]

Hi!

On Fri, Apr 05, 2024 at 04:06:01AM +0200, Hans-Peter Nilsson wrote:
> The xpassing change in generated code was as follows, at
> r14-9788-gb7bd2ec73d66f7 (where I locally applied a revert
> to verify that this suspect was the cause).  That was so
> much of an improvement that I had to share it!  Worth the
> testsuite churn anyway. :)
> 
> Segher, if you end up reverting r14-9692-g839bc42772ba7a (as
> unfortunately seems not unlikely), then please also revert this
> commit: r14-9799-g4c8b3600c4856f7915281ae3ff4d97271c83a540.

I won't revert it, it fixes an actual bug.  Not a regression no, but a
very serious longstanding problem.

We have accidentally done a limited version of a feature requested for
more than 20 years now, "UNCSE".  I'll do this for just combine (instead
of as a separate pass, lots of issues there with pass ordering, results
could be better though) in GCC 15.  This really is a stage 1 thing
though!

Any testcase that relies on something that combine does not promise and
that can not reasonably be expected to always hold is *buggy*.

The combine-2-2 testcase (that I wrote myself) isn't very good, and
should be replaced by something that is much more clearly a 2->2
combination, instead of 1->1 with context.

All (target-specific) new testsuite failures are just like that: bad
testcases!

So no, no reversion.

> That's the only test that's improved to the point of
> affecting test-patterns.  E.g. pr93372-5.c (which references
> pr93372-2.c) is also improved, though it retains a redundant
> compare insn.  (PR 93372 was about regressions from the cc0
> representation; not further improvement like here, thus it's
> not tagged.  Though, I did not double-check whether this
> actually *was* a regression from cc0.)

Interesting that this improved tests for you.  Huh.  Do you have an
explanation how this happened?  I suspect that as uaual it is just a
side effect of random factors: combine is opportunistic, always does the
first change it thinks good, not considering what this then does for
other possible combinations; it is greedy.  It would be nice to see
written out what happens in this example though :-)


Segher

[REVERTED] testsuite/gcc.target/cris/pr93372-2.c: Handle xpass from combine improvement

[Hans-Peter Nilsson]

> Date: Tue, 9 Apr 2024 15:18:10 -0500
> From: Segher Boessenkool <segher@kernel.crashing.org>

> All (target-specific) new testsuite failures are just like that: bad
> testcases!

With a touch of bad assumptions by port-specific code, no
doubt.  Maybe also rtx costs including my pet peeve, the
default implementation of insn_costs (the one that doesn't
look at the destination of setters and which when you try
fixing it, pulls you down a rabbit-hole of cost-related
regressions that even Bernd S. backed away from).

> So no, no reversion.

(...)

> > That's the only test that's improved to the point of
> > affecting test-patterns.  E.g. pr93372-5.c (which references
> > pr93372-2.c) is also improved, though it retains a redundant
> > compare insn.  (PR 93372 was about regressions from the cc0
> > representation; not further improvement like here, thus it's
> > not tagged.  Though, I did not double-check whether this
> > actually *was* a regression from cc0.)
> 
> Interesting that this improved tests for you.  Huh.  Do you have an
> explanation how this happened?

Just a hunch: less combine churn (more straightforward code)
made cmpelim's job easier, same thing you wrote in order
words:

>  I suspect that as uaual it is just a
> side effect of random factors: combine is opportunistic, always does the
> first change it thinks good, not considering what this then does for
> other possible combinations; it is greedy.  It would be nice to see
> written out what happens in this example though :-)

Yes it would, but I have other things on my plate.  Besides,
it's your patch, can't rob you of the fun.

I committed the revert below, but hope to re-apply
(re-revert) it in stage 1, when as per Richard B's message
the combine improvement will reappear.

brgds, H-P

-- >8 --
From: Hans-Peter Nilsson <hp@axis.com>
Date: Wed, 10 Apr 2024 17:24:10 +0200
Subject: [PATCH] Revert "testsuite/gcc.target/cris/pr93372-2.c: Handle xpass
 from combine improvement"

This reverts commit 4c8b3600c4856f7915281ae3ff4d97271c83a540.
---
 gcc/testsuite/gcc.target/cris/pr93372-2.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/gcc/testsuite/gcc.target/cris/pr93372-2.c b/gcc/testsuite/gcc.target/cris/pr93372-2.c
index 2ef6471a990b..912069c018d5 100644
--- a/gcc/testsuite/gcc.target/cris/pr93372-2.c
+++ b/gcc/testsuite/gcc.target/cris/pr93372-2.c
@@ -1,20 +1,19 @@
 /* Check that eliminable compare-instructions are eliminated. */
 /* { dg-do compile } */
 /* { dg-options "-O2" } */
-/* { dg-final { scan-assembler-not "\tcmp|\ttest" } } */
-/* { dg-final { scan-assembler-not "\tnot" } } */
-/* { dg-final { scan-assembler-not "\tlsr" } } */
-/* We should get just one move, storing the result into *d.  */
-/* { dg-final { scan-assembler-times "\tmove" 1 } } */
+/* { dg-final { scan-assembler-not "\tcmp|\ttest" { xfail *-*-* } } } */
+/* { dg-final { scan-assembler-not "\tnot" { xfail cc0 } } } */
+/* { dg-final { scan-assembler-not "\tlsr" { xfail cc0 } } } */
 
 int f(int a, int b, int *d)
 {
   int c = a - b;
 
-  /* We used to get a cmp.d with the original operands here. */
+  /* Whoops!  We get a cmp.d with the original operands here. */
   *d = (c == 0);
 
-  /* We used to get a suboptimal sequence, but now we get the optimal "sge"
-     (a.k.a "spl") re-using flags from the subtraction. */
+  /* Whoops!  While we don't get a test.d for the result here for cc0,
+     we get a sequence of insns: a move, a "not" and a shift of the
+     subtraction-result, where a simple "spl" would have done. */
   return c >= 0;
 }
-- 
2.30.2



brgds, H-P

[COMMITTED] Revert "Revert "testsuite/gcc.target/cris/pr93372-2.c: Handle xpass from combine improvement"" combine improvement

[Hans-Peter Nilsson]

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Thu, 11 Apr 2024 01:16:32 +0200

I committed this revert of a revert, as r15-311, as the
prerequisite was also revert-reverted, in r15-268.

-- >8 --
This reverts commit 39f81924d88e3cc197fc3df74204c9b5e01e12f7.
---
 gcc/testsuite/gcc.target/cris/pr93372-2.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/gcc/testsuite/gcc.target/cris/pr93372-2.c b/gcc/testsuite/gcc.target/cris/pr93372-2.c
index 912069c018d5..2ef6471a990b 100644
--- a/gcc/testsuite/gcc.target/cris/pr93372-2.c
+++ b/gcc/testsuite/gcc.target/cris/pr93372-2.c
@@ -1,19 +1,20 @@
 /* Check that eliminable compare-instructions are eliminated. */
 /* { dg-do compile } */
 /* { dg-options "-O2" } */
-/* { dg-final { scan-assembler-not "\tcmp|\ttest" { xfail *-*-* } } } */
-/* { dg-final { scan-assembler-not "\tnot" { xfail cc0 } } } */
-/* { dg-final { scan-assembler-not "\tlsr" { xfail cc0 } } } */
+/* { dg-final { scan-assembler-not "\tcmp|\ttest" } } */
+/* { dg-final { scan-assembler-not "\tnot" } } */
+/* { dg-final { scan-assembler-not "\tlsr" } } */
+/* We should get just one move, storing the result into *d.  */
+/* { dg-final { scan-assembler-times "\tmove" 1 } } */
 
 int f(int a, int b, int *d)
 {
   int c = a - b;
 
-  /* Whoops!  We get a cmp.d with the original operands here. */
+  /* We used to get a cmp.d with the original operands here. */
   *d = (c == 0);
 
-  /* Whoops!  While we don't get a test.d for the result here for cc0,
-     we get a sequence of insns: a move, a "not" and a shift of the
-     subtraction-result, where a simple "spl" would have done. */
+  /* We used to get a suboptimal sequence, but now we get the optimal "sge"
+     (a.k.a "spl") re-using flags from the subtraction. */
   return c >= 0;
 }
-- 
2.30.2

brgds, H-P