public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] aarch64: Don't assert recog success in ldp/stp pass [PR113114]
@ 2024-01-19 14:00 Alex Coplan
  2024-01-22 18:55 ` Richard Sandiford
  0 siblings, 1 reply; 2+ messages in thread
From: Alex Coplan @ 2024-01-19 14:00 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Earnshaw, Richard Sandiford, Kyrylo Tkachov

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

Hi,

The PR shows two different cases where try_promote_writeback produces an
RTL pattern which isn't recognized.  Currently this leads to an ICE, as
we assert recog success, but I think it's better just to back out of the
changes gracefully if recog fails (as we do in the main fuse_pair case).

In theory since we check the ranges here recog shouldn't fail (which is
why I had the assert in the first place), but the PR shows an edge case
in the patterns where if we form a pre-writeback pair where the
writeback offset is exactly -S, where S is the size in bytes of one
transfer register, we fail to match the expected pattern as the patterns
look explicitly for plus operands in the mems.  I think fixing this
would require adding at least four new special-case patterns to
aarch64.md for what doesn't seem to be a particularly useful variant of
the insns.  Even if we were to do that, I think it would be GCC 15
material, and it's better to just punt for GCC 14.

The ILP32 case in the PR is a bit different, as that shows us trying to
combine a pair with DImode base register operands in the mems together
with an SImode trailing update of the base register.  This leads to us
forming an RTL pattern which references the base register in both SImode
and DImode, which also fails to recog.  Again, I think it's best just to
take the missed optimization for now.  If we really want to make this
(try_promote_writeback) work for ILP32, we can try to do it for GCC 15.

Bootstrapped/regtested on aarch64-linux-gnu (with/without passes
enabled), OK for trunk?

Thanks,
Alex

gcc/ChangeLog:

	PR target/113114
	* config/aarch64/aarch64-ldp-fusion.cc (try_promote_writeback):
	Don't assert recog success, just punt if the writeback pair
	isn't recognized.

gcc/testsuite/ChangeLog:

	PR target/113114
	* gcc.c-torture/compile/pr113114.c: New test.
	* gcc.target/aarch64/pr113114.c: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1726 bytes --]

diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
index 689a8c884bd..19142153f41 100644
--- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
+++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
@@ -2672,7 +2672,15 @@ try_promote_writeback (insn_info *insn)
   for (unsigned i = 0; i < ARRAY_SIZE (changes); i++)
     gcc_assert (rtl_ssa::restrict_movement_ignoring (*changes[i], is_changing));
 
-  gcc_assert (rtl_ssa::recog_ignoring (attempt, pair_change, is_changing));
+  if (!rtl_ssa::recog_ignoring (attempt, pair_change, is_changing))
+    {
+      if (dump_file)
+	fprintf (dump_file, "i%d: recog failed on wb pair, bailing out\n",
+		 insn->uid ());
+      cancel_changes (0);
+      return;
+    }
+
   gcc_assert (crtl->ssa->verify_insn_changes (changes));
   confirm_change_group ();
   crtl->ssa->change_insns (changes);
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113114.c b/gcc/testsuite/gcc.c-torture/compile/pr113114.c
new file mode 100644
index 00000000000..978e594eb3d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr113114.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-funroll-loops" } */
+float val[128];
+float x;
+void bar() {
+  int i = 55;
+  for (; i >= 0; --i)
+    x += val[i];
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/pr113114.c b/gcc/testsuite/gcc.target/aarch64/pr113114.c
new file mode 100644
index 00000000000..5b0383c2435
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr113114.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-mabi=ilp32 -O -mearly-ldp-fusion -mlate-ldp-fusion" } */
+void foo_n(double *a) {
+  int i = 1;
+  for (; i < (int)foo_n; i++)
+    a[i] = a[i - 1] + a[i + 1] * a[i];
+}

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] aarch64: Don't assert recog success in ldp/stp pass [PR113114]
  2024-01-19 14:00 [PATCH] aarch64: Don't assert recog success in ldp/stp pass [PR113114] Alex Coplan
@ 2024-01-22 18:55 ` Richard Sandiford
  0 siblings, 0 replies; 2+ messages in thread
From: Richard Sandiford @ 2024-01-22 18:55 UTC (permalink / raw)
  To: Alex Coplan; +Cc: gcc-patches, Richard Earnshaw, Kyrylo Tkachov

Alex Coplan <alex.coplan@arm.com> writes:
> Hi,
>
> The PR shows two different cases where try_promote_writeback produces an
> RTL pattern which isn't recognized.  Currently this leads to an ICE, as
> we assert recog success, but I think it's better just to back out of the
> changes gracefully if recog fails (as we do in the main fuse_pair case).
>
> In theory since we check the ranges here recog shouldn't fail (which is
> why I had the assert in the first place), but the PR shows an edge case
> in the patterns where if we form a pre-writeback pair where the
> writeback offset is exactly -S, where S is the size in bytes of one
> transfer register, we fail to match the expected pattern as the patterns
> look explicitly for plus operands in the mems.  I think fixing this
> would require adding at least four new special-case patterns to
> aarch64.md for what doesn't seem to be a particularly useful variant of
> the insns.  Even if we were to do that, I think it would be GCC 15
> material, and it's better to just punt for GCC 14.
>
> The ILP32 case in the PR is a bit different, as that shows us trying to
> combine a pair with DImode base register operands in the mems together
> with an SImode trailing update of the base register.  This leads to us
> forming an RTL pattern which references the base register in both SImode
> and DImode, which also fails to recog.  Again, I think it's best just to
> take the missed optimization for now.  If we really want to make this
> (try_promote_writeback) work for ILP32, we can try to do it for GCC 15.
>
> Bootstrapped/regtested on aarch64-linux-gnu (with/without passes
> enabled), OK for trunk?
>
> Thanks,
> Alex
>
> gcc/ChangeLog:
>
> 	PR target/113114
> 	* config/aarch64/aarch64-ldp-fusion.cc (try_promote_writeback):
> 	Don't assert recog success, just punt if the writeback pair
> 	isn't recognized.
>
> gcc/testsuite/ChangeLog:
>
> 	PR target/113114
> 	* gcc.c-torture/compile/pr113114.c: New test.
> 	* gcc.target/aarch64/pr113114.c: New test.

OK, thanks.

Richard

>
> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> index 689a8c884bd..19142153f41 100644
> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> @@ -2672,7 +2672,15 @@ try_promote_writeback (insn_info *insn)
>    for (unsigned i = 0; i < ARRAY_SIZE (changes); i++)
>      gcc_assert (rtl_ssa::restrict_movement_ignoring (*changes[i], is_changing));
>  
> -  gcc_assert (rtl_ssa::recog_ignoring (attempt, pair_change, is_changing));
> +  if (!rtl_ssa::recog_ignoring (attempt, pair_change, is_changing))
> +    {
> +      if (dump_file)
> +	fprintf (dump_file, "i%d: recog failed on wb pair, bailing out\n",
> +		 insn->uid ());
> +      cancel_changes (0);
> +      return;
> +    }
> +
>    gcc_assert (crtl->ssa->verify_insn_changes (changes));
>    confirm_change_group ();
>    crtl->ssa->change_insns (changes);
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113114.c b/gcc/testsuite/gcc.c-torture/compile/pr113114.c
> new file mode 100644
> index 00000000000..978e594eb3d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr113114.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-funroll-loops" } */
> +float val[128];
> +float x;
> +void bar() {
> +  int i = 55;
> +  for (; i >= 0; --i)
> +    x += val[i];
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr113114.c b/gcc/testsuite/gcc.target/aarch64/pr113114.c
> new file mode 100644
> index 00000000000..5b0383c2435
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr113114.c
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mabi=ilp32 -O -mearly-ldp-fusion -mlate-ldp-fusion" } */
> +void foo_n(double *a) {
> +  int i = 1;
> +  for (; i < (int)foo_n; i++)
> +    a[i] = a[i - 1] + a[i + 1] * a[i];
> +}

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-01-22 18:55 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-19 14:00 [PATCH] aarch64: Don't assert recog success in ldp/stp pass [PR113114] Alex Coplan
2024-01-22 18:55 ` Richard Sandiford

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).