| Message ID | ZaqAmMhtClLBf838@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Don't assert recog success in ldp/stp pass [PR113114] | expand |
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]; > +}
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]; +}