[pushed] rtl-ssa: Fix removal of order_nodes [PR115929]
Checks
Commit Message
order_nodes are used to implement ordered comparisons between
two insns with the same program point number. remove_insn would
remove an order_node from its splay tree, but didn't remove it
from the insn. This caused confusion if the insn was later
reinserted somewhere else that also needed an order_node.
Tested on aarch64-linux-gnu & x86_64-linux-gnu. Pushed as obvious.
Richard
gcc/
PR rtl-optimization/115929
* rtl-ssa/insns.cc (function_info::remove_insn): Remove an
order_node from the instruction as well as from the splay tree.
gcc/testsuite/
PR rtl-optimization/115929
* gcc.dg/torture/pr115929-1.c: New test.
---
gcc/rtl-ssa/insns.cc | 5 ++-
gcc/testsuite/gcc.dg/torture/pr115929-1.c | 45 +++++++++++++++++++++++
2 files changed, 49 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/torture/pr115929-1.c
Comments
On 7/16/24 8:34 AM, Richard Sandiford wrote:
> order_nodes are used to implement ordered comparisons between
> two insns with the same program point number. remove_insn would
> remove an order_node from its splay tree, but didn't remove it
> from the insn. This caused confusion if the insn was later
> reinserted somewhere else that also needed an order_node.
>
> Tested on aarch64-linux-gnu & x86_64-linux-gnu. Pushed as obvious.
>
> Richard
>
>
> gcc/
> PR rtl-optimization/115929
> * rtl-ssa/insns.cc (function_info::remove_insn): Remove an
> order_node from the instruction as well as from the splay tree.
>
> gcc/testsuite/
> PR rtl-optimization/115929
> * gcc.dg/torture/pr115929-1.c: New test.
OK. I do find myself wondering if you should just be pushing these
patches. You know this code far better than anyone.
jeff
Am 16.07.24 um 16:34 schrieb Richard Sandiford:
> order_nodes are used to implement ordered comparisons between
> two insns with the same program point number. remove_insn would
> remove an order_node from its splay tree, but didn't remove it
> from the insn. This caused confusion if the insn was later
> reinserted somewhere else that also needed an order_node.
>
> Tested on aarch64-linux-gnu & x86_64-linux-gnu. Pushed as obvious.
>
> Richard
>
>
> gcc/
> PR rtl-optimization/115929
> * rtl-ssa/insns.cc (function_info::remove_insn): Remove an
> order_node from the instruction as well as from the splay tree.
>
> gcc/testsuite/
> PR rtl-optimization/115929
> * gcc.dg/torture/pr115929-1.c: New test.
> ---
> gcc/rtl-ssa/insns.cc | 5 ++-
> gcc/testsuite/gcc.dg/torture/pr115929-1.c | 45 +++++++++++++++++++++++
> 2 files changed, 49 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr115929-1.c
>
> diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc
> index 7e26bfd978f..bc30734df89 100644
> --- a/gcc/rtl-ssa/insns.cc
> +++ b/gcc/rtl-ssa/insns.cc
> @@ -393,7 +393,10 @@ void
> function_info::remove_insn (insn_info *insn)
> {
> if (insn_info::order_node *order = insn->get_order_node ())
> - insn_info::order_splay_tree::remove_node (order);
> + {
> + insn_info::order_splay_tree::remove_node (order);
> + insn->remove_note (order);
> + }
>
> if (auto *note = insn->find_note<insn_call_clobbers_note> ())
> {
> diff --git a/gcc/testsuite/gcc.dg/torture/pr115929-1.c b/gcc/testsuite/gcc.dg/torture/pr115929-1.c
> new file mode 100644
> index 00000000000..19b831ab99e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr115929-1.c
> @@ -0,0 +1,45 @@
> +/* { dg-require-effective-target lp64 } */
> +/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch" } */
Hi, this fails on machines that don't support scheduling:
cc1: warning: instruction scheduling not supported on this target machine
FAIL: gcc.dg/torture/pr115929-2.c -O0 (test for excess errors)
Excess errors:
cc1: warning: instruction scheduling not supported on this target machine
Better do
/* { dg-require-effective-target scheduling } */
Johann
> +
> +int printf(const char *, ...);
> +int a[6], b, c;
> +char d, l;
> +struct {
> + char e;
> + int f;
> + int : 8;
> + long g;
> + long h;
> +} i[1][9] = {0};
> +unsigned j;
> +void n(char p) { b = b >> 8 ^ a[b ^ p]; }
> +int main() {
> + int k, o;
> + while (b) {
> + k = 0;
> + for (; k < 9; k++) {
> + b = b ^ a[l];
> + n(j);
> + if (o)
> + printf(&d);
> + long m = i[c][k].f;
> + b = b >> 8 ^ a[l];
> + n(m >> 32);
> + n(m);
> + if (o)
> + printf("%d", d);
> + b = b >> 8 ^ l;
> + n(2);
> + n(0);
> + if (o)
> + printf(&d);
> + b = b ^ a[l];
> + n(i[c][k].g >> 2);
> + n(i[c][k].g);
> + if (o)
> + printf(&d);
> + printf("%d", i[c][k].f);
> + }
> + }
> + return 0;
> +}
@@ -393,7 +393,10 @@ void
function_info::remove_insn (insn_info *insn)
{
if (insn_info::order_node *order = insn->get_order_node ())
- insn_info::order_splay_tree::remove_node (order);
+ {
+ insn_info::order_splay_tree::remove_node (order);
+ insn->remove_note (order);
+ }
if (auto *note = insn->find_note<insn_call_clobbers_note> ())
{
new file mode 100644
@@ -0,0 +1,45 @@
+/* { dg-require-effective-target lp64 } */
+/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch" } */
+
+int printf(const char *, ...);
+int a[6], b, c;
+char d, l;
+struct {
+ char e;
+ int f;
+ int : 8;
+ long g;
+ long h;
+} i[1][9] = {0};
+unsigned j;
+void n(char p) { b = b >> 8 ^ a[b ^ p]; }
+int main() {
+ int k, o;
+ while (b) {
+ k = 0;
+ for (; k < 9; k++) {
+ b = b ^ a[l];
+ n(j);
+ if (o)
+ printf(&d);
+ long m = i[c][k].f;
+ b = b >> 8 ^ a[l];
+ n(m >> 32);
+ n(m);
+ if (o)
+ printf("%d", d);
+ b = b >> 8 ^ l;
+ n(2);
+ n(0);
+ if (o)
+ printf(&d);
+ b = b ^ a[l];
+ n(i[c][k].g >> 2);
+ n(i[c][k].g);
+ if (o)
+ printf(&d);
+ printf("%d", i[c][k].f);
+ }
+ }
+ return 0;
+}