diff mbox series

late-combine: Preserve INSN_CODE when modifying notes [PR116343]

Message ID mptsev69ntb.fsf@arm.com
State New
Headers show
Series late-combine: Preserve INSN_CODE when modifying notes [PR116343] | expand

Commit Message

Richard Sandiford Aug. 15, 2024, 8:45 a.m. UTC 
When it removes a definition, late-combine tries to update all
uses in notes.  It does this using the same insn_propagation class
that it uses for patterns.

However, insn_propagation uses validate_change, which in turn
resets the INSN_CODE.  This is inefficient in the best case,
since it forces the pattern to be rerecognised even though
changing a note can't affect the INSN_CODE.  But in the PR
it's a correctness problem: resetting INSN_CODE means we lose
the NOOP_INSN_MOVE_CODE, which in turn means that rtl-ssa doesn't
queue it for deletion.

This patch adds a routine specifically for propagating into notes.
A belt-and-braces fix would be to rerecognise noop moves in
function_info::change_insns, but I can't think of a good reason
why that would be necessary, and it could paper over latent bugs.

Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?

Richard


gcc/
	PR testsuite/116343
	* recog.h (insn_propagation::apply_to_note): Declare.
	* recog.cc (insn_propagation::apply_to_note): New function.
	* late-combine.cc (insn_combination::substitute_note): Use
	apply_to_note instead of apply_to_rvalue.
	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Improve
	dumping of costs for noop moves.

gcc/testsuite/
	PR testsuite/116343
	* gcc.dg/torture/pr116343.c: New test.
---
 gcc/late-combine.cc                     |  2 +-
 gcc/recog.cc                            | 13 +++++++++++++
 gcc/recog.h                             |  1 +
 gcc/rtl-ssa/changes.cc                  |  5 ++++-
 gcc/testsuite/gcc.dg/torture/pr116343.c | 18 ++++++++++++++++++
 5 files changed, 37 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr116343.c

Comments

Jeff Law Aug. 15, 2024, 3 p.m. UTC | #1 
On 8/15/24 2:45 AM, Richard Sandiford wrote:
> When it removes a definition, late-combine tries to update all
> uses in notes.  It does this using the same insn_propagation class
> that it uses for patterns.
> 
> However, insn_propagation uses validate_change, which in turn
> resets the INSN_CODE.  This is inefficient in the best case,
> since it forces the pattern to be rerecognised even though
> changing a note can't affect the INSN_CODE.  But in the PR
> it's a correctness problem: resetting INSN_CODE means we lose
> the NOOP_INSN_MOVE_CODE, which in turn means that rtl-ssa doesn't
> queue it for deletion.
> 
> This patch adds a routine specifically for propagating into notes.
> A belt-and-braces fix would be to rerecognise noop moves in
> function_info::change_insns, but I can't think of a good reason
> why that would be necessary, and it could paper over latent bugs.
> 
> Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?
> 
> Richard
> 
> 
> gcc/
> 	PR testsuite/116343
> 	* recog.h (insn_propagation::apply_to_note): Declare.
> 	* recog.cc (insn_propagation::apply_to_note): New function.
> 	* late-combine.cc (insn_combination::substitute_note): Use
> 	apply_to_note instead of apply_to_rvalue.
> 	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Improve
> 	dumping of costs for noop moves.
Having fought problems with changing INSN_CODEs and rerecognition, I've 
wondered if we really should have an API for that to always put things 
back the way they were.  Though I guess in this case and the one I 
looked at likely wouldn't want to use the same API.  So perhaps not a 
great idea.


OK

jeff
Georg-Johann Lay Aug. 23, 2024, 12:02 p.m. UTC | #2 
Am 15.08.24 um 10:45 schrieb Richard Sandiford:
> When it removes a definition, late-combine tries to update all
> uses in notes.  It does this using the same insn_propagation class
> that it uses for patterns.
> 
> However, insn_propagation uses validate_change, which in turn
> resets the INSN_CODE.  This is inefficient in the best case,
> since it forces the pattern to be rerecognised even though
> changing a note can't affect the INSN_CODE.  But in the PR
> it's a correctness problem: resetting INSN_CODE means we lose
> the NOOP_INSN_MOVE_CODE, which in turn means that rtl-ssa doesn't
> queue it for deletion.
> 
> This patch adds a routine specifically for propagating into notes.
> A belt-and-braces fix would be to rerecognise noop moves in
> function_info::change_insns, but I can't think of a good reason
> why that would be necessary, and it could paper over latent bugs.
> 
> Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?
> 
> Richard
> 
> 
> gcc/
> 	PR testsuite/116343
> 	* recog.h (insn_propagation::apply_to_note): Declare.
> 	* recog.cc (insn_propagation::apply_to_note): New function.
> 	* late-combine.cc (insn_combination::substitute_note): Use
> 	apply_to_note instead of apply_to_rvalue.
> 	* rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Improve
> 	dumping of costs for noop moves.
> 
> gcc/testsuite/
> 	PR testsuite/116343
> 	* gcc.dg/torture/pr116343.c: New test.
> ---
>   gcc/late-combine.cc                     |  2 +-
>   gcc/recog.cc                            | 13 +++++++++++++
>   gcc/recog.h                             |  1 +
>   gcc/rtl-ssa/changes.cc                  |  5 ++++-
>   gcc/testsuite/gcc.dg/torture/pr116343.c | 18 ++++++++++++++++++
>   5 files changed, 37 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/torture/pr116343.c
> 
> diff --git a/gcc/late-combine.cc b/gcc/late-combine.cc
> index 2b62e2956ed..1d81b386c3d 100644
> --- a/gcc/late-combine.cc
> +++ b/gcc/late-combine.cc
> @@ -338,7 +338,7 @@ insn_combination::substitute_note (insn_info *use_insn, rtx note,
>         || REG_NOTE_KIND (note) == REG_EQUIV)
>       {
>         insn_propagation prop (use_insn->rtl (), m_dest, m_src);
> -      return (prop.apply_to_rvalue (&XEXP (note, 0))
> +      return (prop.apply_to_note (&XEXP (note, 0))
>   	      && (can_propagate || prop.num_replacements == 0));
>       }
>     return true;
> diff --git a/gcc/recog.cc b/gcc/recog.cc
> index 23e4820180f..615aaabc551 100644
> --- a/gcc/recog.cc
> +++ b/gcc/recog.cc
> @@ -1469,6 +1469,19 @@ insn_propagation::apply_to_rvalue (rtx *loc)
>     return res;
>   }
>   
> +/* Like apply_to_rvalue, but specifically for the case where *LOC is in
> +   a note.  This never changes the INSN_CODE.  */
> +
> +bool
> +insn_propagation::apply_to_note (rtx *loc)
> +{
> +  auto old_code = INSN_CODE (insn);
> +  bool res = apply_to_rvalue (loc);
> +  if (INSN_CODE (insn) != old_code)
> +    INSN_CODE (insn) = old_code;
> +  return res;
> +}
> +
>   /* Check whether INSN matches a specific alternative of an .md pattern.  */
>   
>   bool
> diff --git a/gcc/recog.h b/gcc/recog.h
> index 87a5803dec0..1dccce78ba4 100644
> --- a/gcc/recog.h
> +++ b/gcc/recog.h
> @@ -121,6 +121,7 @@ public:
>     insn_propagation (rtx_insn *, rtx, rtx, bool = true);
>     bool apply_to_pattern (rtx *);
>     bool apply_to_rvalue (rtx *);
> +  bool apply_to_note (rtx *);
>   
>     /* Return true if we should accept a substitution into the address of
>        memory expression MEM.  Undoing changes OLD_NUM_CHANGES and up restores
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index a30f000191e..0476296607b 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -228,7 +228,10 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
>         for (const insn_change *change : changes)
>   	if (!change->is_deletion ())
>   	  {
> -	    fprintf (dump_file, " %c %d", sep, change->new_cost);
> +	    if (INSN_CODE (change->rtl ()) == NOOP_MOVE_INSN_CODE)
> +	      fprintf (dump_file, " %c nop", sep);
> +	    else
> +	      fprintf (dump_file, " %c %d", sep, change->new_cost);
>   	    sep = '+';
>   	  }
>         if (weighted_new_cost != 0)
> diff --git a/gcc/testsuite/gcc.dg/torture/pr116343.c b/gcc/testsuite/gcc.dg/torture/pr116343.c
> new file mode 100644
> index 00000000000..ad13f0fc21c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr116343.c
> @@ -0,0 +1,18 @@
> +// { dg-additional-options "-fschedule-insns -fno-thread-jumps -fno-dce" }

Hi, this fails on machines that don't support scheduling:

cc1: warning: instruction scheduling not supported on this target machine

FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
Excess errors:
cc1: warning: instruction scheduling not supported on this target machine

Johann

> +
> +int a, b, c;
> +volatile int d;
> +int e(int f, int g) { return g > 1 ? 1 : f >> g; }
> +int main() {
> +  int *i = &a;
> +  long j[1];
> +  if (a)
> +    while (1) {
> +      a ^= 1;
> +      if (*i)
> +        while (1)
> +          ;
> +      b = c && e((d, 1) >= 1, j[0]);
> +    }
> +  return 0;
> +}
Jeff Law Aug. 23, 2024, 2:48 p.m. UTC | #3 
On 8/23/24 6:02 AM, Georg-Johann Lay wrote:

> 
> Hi, this fails on machines that don't support scheduling:
> 
> cc1: warning: instruction scheduling not supported on this target machine
> 
> FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
> Excess errors:
> cc1: warning: instruction scheduling not supported on this target machine
Two paths make sense to me.

First we could add a -w to the flags in the relevant testcases to 
suppress the warning.

Second we could just eliminate the warning completely.  The warning may 
have made sense in the run-up to gcc-2 when we added the instruction 
scheduler.  But we're 30 years past that point.

I'd support either approach.

jeff
Richard Biener Aug. 23, 2024, 3:45 p.m. UTC | #4 
> Am 23.08.2024 um 16:49 schrieb Jeff Law <jeffreyalaw@gmail.com>:
> 
> 
> 
>> On 8/23/24 6:02 AM, Georg-Johann Lay wrote:
>> 
>> Hi, this fails on machines that don't support scheduling:
>> cc1: warning: instruction scheduling not supported on this target machine
>> FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
>> Excess errors:
>> cc1: warning: instruction scheduling not supported on this target machine
> Two paths make sense to me.
> 
> First we could add a -w to the flags in the relevant testcases to suppress the warning.
> 
> Second we could just eliminate the warning completely.  The warning may have made sense in the run-up to gcc-2 when we added the instruction scheduler.  But we're 30 years past that point.
> 
> I'd support either approach.

I think there’s an effective target for insn scheduling 

> jeff
Jeff Law Aug. 23, 2024, 3:47 p.m. UTC | #5 
On 8/23/24 9:45 AM, Richard Biener wrote:
> 
> 
>> Am 23.08.2024 um 16:49 schrieb Jeff Law <jeffreyalaw@gmail.com>:
>>
>> 
>>
>>> On 8/23/24 6:02 AM, Georg-Johann Lay wrote:
>>>
>>> Hi, this fails on machines that don't support scheduling:
>>> cc1: warning: instruction scheduling not supported on this target machine
>>> FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
>>> Excess errors:
>>> cc1: warning: instruction scheduling not supported on this target machine
>> Two paths make sense to me.
>>
>> First we could add a -w to the flags in the relevant testcases to suppress the warning.
>>
>> Second we could just eliminate the warning completely.  The warning may have made sense in the run-up to gcc-2 when we added the instruction scheduler.  But we're 30 years past that point.
>>
>> I'd support either approach.
> 
> I think there’s an effective target for insn scheduling
If so, that's fine by me as well.

jeff
Georg-Johann Lay Aug. 23, 2024, 3:59 p.m. UTC | #6 
Am 23.08.24 um 17:45 schrieb Richard Biener:
>> Am 23.08.2024 um 16:49 schrieb Jeff Law <jeffreyalaw@gmail.com>:
>>> On 8/23/24 6:02 AM, Georg-Johann Lay wrote:
>>>
>>> Hi, this fails on machines that don't support scheduling:
>>> cc1: warning: instruction scheduling not supported on this target machine
>>> FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
>>> Excess errors:
>>> cc1: warning: instruction scheduling not supported on this target machine
>> Two paths make sense to me.
>>
>> First we could add a -w to the flags in the relevant testcases to suppress the warning.
>>
>> Second we could just eliminate the warning completely.  The warning may have made sense in the run-up to gcc-2 when we added the instruction scheduler.  But we're 30 years past that point.
>>
>> I'd support either approach.
> 
> I think there’s an effective target for insn scheduling

Ya,


/* { dg-require-effective-target scheduling } */

Johann
Georg-Johann Lay Aug. 24, 2024, 8:35 a.m. UTC | #7 
Am 23.08.24 um 17:47 schrieb Jeff Law:
> On 8/23/24 9:45 AM, Richard Biener wrote:
>>> Am 23.08.2024 um 16:49 schrieb Jeff Law <jeffreyalaw@gmail.com>:
>>>> On 8/23/24 6:02 AM, Georg-Johann Lay wrote:
>>>>
>>>> Hi, this fails on machines that don't support scheduling:
>>>> cc1: warning: instruction scheduling not supported on this target 
>>>> machine
>>>> FAIL: gcc.dg/torture/pr116343.c   -O0  (test for excess errors)
>>>> Excess errors:
>>>> cc1: warning: instruction scheduling not supported on this target 
>>>> machine
>>> Two paths make sense to me.
>>>
>>> First we could add a -w to the flags in the relevant testcases to 
>>> suppress the warning.
>>>
>>> Second we could just eliminate the warning completely.  The warning 
>>> may have made sense in the run-up to gcc-2 when we added the 
>>> instruction scheduler.  But we're 30 years past that point.
>>>
>>> I'd support either approach.
>>
>> I think there’s an effective target for insn scheduling
> If so, that's fine by me as well.
> 
> jeff

Ok, applied this one,

Johann

     testsuite: Add dg-require-effective-target scheduling for some 
tests that set -fschedule-insns.

     gcc/testsuite/
             * gcc.dg/torture/pr115929-2.c: Add 
dg-require-effective-target scheduling.
             * gcc.dg/torture/pr116343.c: Same.

diff --git a/gcc/testsuite/gcc.dg/torture/pr115929-2.c 
b/gcc/testsuite/gcc.dg/torture/pr115929-2.c
index c8473a74da6..02496d54d79 100644
--- a/gcc/testsuite/gcc.dg/torture/pr115929-2.c
+++ b/gcc/testsuite/gcc.dg/torture/pr115929-2.c
@@ -1,4 +1,5 @@
  /* { dg-additional-options "-fschedule-insns" } */
+/* { dg-require-effective-target scheduling } */

  int a, b, c, d, e, f;
  int main() {
diff --git a/gcc/testsuite/gcc.dg/torture/pr116343.c 
b/gcc/testsuite/gcc.dg/torture/pr116343.c
index ad13f0fc21c..287a09707ec 100644
--- a/gcc/testsuite/gcc.dg/torture/pr116343.c
+++ b/gcc/testsuite/gcc.dg/torture/pr116343.c
@@ -1,4 +1,5 @@
  // { dg-additional-options "-fschedule-insns -fno-thread-jumps -fno-dce" }
+/* { dg-require-effective-target scheduling } */

  int a, b, c;
  volatile int d;
diff mbox series

Patch

diff --git a/gcc/late-combine.cc b/gcc/late-combine.cc
index 2b62e2956ed..1d81b386c3d 100644
--- a/gcc/late-combine.cc
+++ b/gcc/late-combine.cc
@@ -338,7 +338,7 @@  insn_combination::substitute_note (insn_info *use_insn, rtx note,
       || REG_NOTE_KIND (note) == REG_EQUIV)
     {
       insn_propagation prop (use_insn->rtl (), m_dest, m_src);
-      return (prop.apply_to_rvalue (&XEXP (note, 0))
+      return (prop.apply_to_note (&XEXP (note, 0))
 	      && (can_propagate || prop.num_replacements == 0));
     }
   return true;
diff --git a/gcc/recog.cc b/gcc/recog.cc
index 23e4820180f..615aaabc551 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -1469,6 +1469,19 @@  insn_propagation::apply_to_rvalue (rtx *loc)
   return res;
 }
 
+/* Like apply_to_rvalue, but specifically for the case where *LOC is in
+   a note.  This never changes the INSN_CODE.  */
+
+bool
+insn_propagation::apply_to_note (rtx *loc)
+{
+  auto old_code = INSN_CODE (insn);
+  bool res = apply_to_rvalue (loc);
+  if (INSN_CODE (insn) != old_code)
+    INSN_CODE (insn) = old_code;
+  return res;
+}
+
 /* Check whether INSN matches a specific alternative of an .md pattern.  */
 
 bool
diff --git a/gcc/recog.h b/gcc/recog.h
index 87a5803dec0..1dccce78ba4 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -121,6 +121,7 @@  public:
   insn_propagation (rtx_insn *, rtx, rtx, bool = true);
   bool apply_to_pattern (rtx *);
   bool apply_to_rvalue (rtx *);
+  bool apply_to_note (rtx *);
 
   /* Return true if we should accept a substitution into the address of
      memory expression MEM.  Undoing changes OLD_NUM_CHANGES and up restores
diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index a30f000191e..0476296607b 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -228,7 +228,10 @@  rtl_ssa::changes_are_worthwhile (array_slice<insn_change *const> changes,
       for (const insn_change *change : changes)
 	if (!change->is_deletion ())
 	  {
-	    fprintf (dump_file, " %c %d", sep, change->new_cost);
+	    if (INSN_CODE (change->rtl ()) == NOOP_MOVE_INSN_CODE)
+	      fprintf (dump_file, " %c nop", sep);
+	    else
+	      fprintf (dump_file, " %c %d", sep, change->new_cost);
 	    sep = '+';
 	  }
       if (weighted_new_cost != 0)
diff --git a/gcc/testsuite/gcc.dg/torture/pr116343.c b/gcc/testsuite/gcc.dg/torture/pr116343.c
new file mode 100644
index 00000000000..ad13f0fc21c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr116343.c
@@ -0,0 +1,18 @@ 
+// { dg-additional-options "-fschedule-insns -fno-thread-jumps -fno-dce" }
+
+int a, b, c;
+volatile int d;
+int e(int f, int g) { return g > 1 ? 1 : f >> g; }
+int main() {
+  int *i = &a;
+  long j[1];
+  if (a)
+    while (1) {
+      a ^= 1;
+      if (*i)
+        while (1)
+          ;
+      b = c && e((d, 1) >= 1, j[0]);
+    }
+  return 0;
+}