public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc r12-808] regcprop: Fix another cprop_hardreg bug [PR100342]
@ 2021-05-15 8:14 Jakub Jelinek
0 siblings, 0 replies; only message in thread
From: Jakub Jelinek @ 2021-05-15 8:14 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:425ad87dcfacbb326d8f448a0f2b4d6b53dcd98f
commit r12-808-g425ad87dcfacbb326d8f448a0f2b4d6b53dcd98f
Author: Jakub Jelinek <jakub@redhat.com>
Date: Sat May 15 10:12:11 2021 +0200
regcprop: Fix another cprop_hardreg bug [PR100342]
On Tue, Jan 19, 2021 at 04:10:33PM +0000, Richard Sandiford via Gcc-patches wrote:
> Ah, ok, thanks for the extra context.
>
> So AIUI the problem when recording xmm2<-di isn't just:
>
> [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
>
> but also that:
>
> [B] partial_subreg_p (vd->e[sr].mode, vd->e[vd->e[sr].oldest_regno].mode)
>
> For example, all registers in this sequence can be part of the same chain:
>
> (set (reg:HI R1) (reg:HI R0))
> (set (reg:SI R2) (reg:SI R1)) // [A]
> (set (reg:DI R3) (reg:DI R2)) // [A]
> (set (reg:SI R4) (reg:SI R[0-3]))
> (set (reg:HI R5) (reg:HI R[0-4]))
>
> But:
>
> (set (reg:SI R1) (reg:SI R0))
> (set (reg:HI R2) (reg:HI R1))
> (set (reg:SI R3) (reg:SI R2)) // [A] && [B]
>
> is problematic because it dips below the precision of the oldest regno
> and then increases again.
>
> When this happens, I guess we have two choices:
>
> (1) what the patch does: treat R3 as the start of a new chain.
> (2) pretend that the copy occured in vd->e[sr].mode instead
> (i.e. copy vd->e[sr].mode to vd->e[dr].mode)
>
> I guess (2) would need to be subject to REG_CAN_CHANGE_MODE_P.
> Maybe the optimisation provided by (2) compared to (1) isn't common
> enough to be worth the complication.
>
> I think we should test [B] as well as [A] though. The pass is set
> up to do some quite elaborate mode changes and I think rejecting
> [A] on its own would make some of the other code redundant.
> It also feels like it should be a seperate “if” or “else if”,
> with its own comment.
Unfortunately, we now have a testcase that shows that testing also [B]
is a problem (unfortunately now latent on the trunk, only reproduces
on 10 and 11 branches).
The comment in the patch tries to list just the interesting instructions,
we have a 64-bit value, copy low 8 bit of those to another register,
copy full 64 bits to another register and then clobber the original register.
Before that (set (reg:DI r14) (const_int ...)) we have a chain
DI r14, QI si, DI bp , that instruction drops the DI r14 from that chain, so
we have QI si, DI bp , si being the oldest_regno.
Next DI si is copied into DI dx. Only the low 8 bits of that are defined,
the rest is unspecified, but we would add DI dx into that same chain at the
end, so QI si, DI bp, DI dx [*]. Next si is overwritten, so the chain is
DI bp, DI dx. And then we see (set (reg:DI dx) (reg:DI bp)) and remove it
as redundant, because we think bp and dx are already equivalent, when in
reality that is true only for the lowpart 8 bits.
I believe the [*] marked step above is where the bug is.
The committed regcprop.c (copy_value) change (but only committed to
trunk/11, not to 10) added
else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
&& partial_subreg_p (vd->e[sr].mode,
vd->e[vd->e[sr].oldest_regno].mode))
return;
and while the first partial_subreg_p call returns true, the second one
doesn't; before the (set (reg:DI r14) (const_int ...)) insn it would be
true and we'd return, but as that reg got clobbered, si became the oldest
regno in the chain and so vd->e[vd->e[sr].oldest_regno].mode is QImode
and vd->e[sr].mode is QImode too, so the second partial_subreg_p is false.
But as the testcase shows, what is the oldest_regno in the chain is
something that changes over time, so relying on it for anything is
problematic, something could have a different oldest_regno and later
on get a different oldest_regno (perhaps with different mode) because
the oldest_regno got overwritten and it can change both ways.
The following patch effectively implements your (2) above.
2021-05-15 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/100342
* regcprop.c (copy_value): When copying a source reg in a wider
mode than it has recorded for the value, adjust recorded destination
mode too or punt if !REG_CAN_CHANGE_MODE_P.
* gcc.target/i386/pr100342.c: New test.
Diff:
---
gcc/regcprop.c | 48 +++++++++-------------
gcc/testsuite/gcc.target/i386/pr100342.c | 70 ++++++++++++++++++++++++++++++++
2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index 02753a12510..7c271e22f47 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -358,34 +358,26 @@ copy_value (rtx dest, rtx src, struct value_data *vd)
else if (sn > hard_regno_nregs (sr, vd->e[sr].mode))
return;
- /* It is not safe to link DEST into the chain if SRC was defined in some
- narrower mode M and if M is also narrower than the mode of the first
- register in the chain. For example:
- (set (reg:DI r1) (reg:DI r0))
- (set (reg:HI r2) (reg:HI r1))
- (set (reg:SI r3) (reg:SI r2)) //Should be a new chain start at r3
- (set (reg:SI r4) (reg:SI r1))
- (set (reg:SI r5) (reg:SI r4))
-
- the upper part of r3 is undefined. If we added it to the chain,
- it may be used to replace r5, which has defined upper bits.
- See PR98694 for details.
-
- [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
- [B] partial_subreg_p (vd->e[sr].mode, vd->e[vd->e[sr].oldest_regno].mode)
- Condition B is added to to catch optimization opportunities of
-
- (set (reg:HI R1) (reg:HI R0))
- (set (reg:SI R2) (reg:SI R1)) // [A]
- (set (reg:DI R3) (reg:DI R2)) // [A]
- (set (reg:SI R4) (reg:SI R[0-3]))
- (set (reg:HI R5) (reg:HI R[0-4]))
-
- in which all registers have only 16 defined bits. */
- else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
- && partial_subreg_p (vd->e[sr].mode,
- vd->e[vd->e[sr].oldest_regno].mode))
- return;
+ /* If a narrower value is copied using wider mode, the upper bits
+ are undefined (could be e.g. a former paradoxical subreg). Signal
+ in that case we've only copied value using the narrower mode.
+ Consider:
+ (set (reg:DI r14) (mem:DI ...))
+ (set (reg:QI si) (reg:QI r14))
+ (set (reg:DI bp) (reg:DI r14))
+ (set (reg:DI r14) (const_int ...))
+ (set (reg:DI dx) (reg:DI si))
+ (set (reg:DI si) (const_int ...))
+ (set (reg:DI dx) (reg:DI bp))
+ The last set is not redundant, while the low 8 bits of dx are already
+ equal to low 8 bits of bp, the other bits are undefined. */
+ else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src)))
+ {
+ if (!REG_CAN_CHANGE_MODE_P (sr, GET_MODE (src), vd->e[sr].mode)
+ || !REG_CAN_CHANGE_MODE_P (dr, vd->e[sr].mode, GET_MODE (dest)))
+ return;
+ set_value_regno (dr, vd->e[sr].mode, vd);
+ }
/* Link DR at the end of the value chain used by SR. */
diff --git a/gcc/testsuite/gcc.target/i386/pr100342.c b/gcc/testsuite/gcc.target/i386/pr100342.c
new file mode 100644
index 00000000000..8e2ec823748
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr100342.c
@@ -0,0 +1,70 @@
+/* PR rtl-optimization/100342 */
+/* { dg-do run { target int128 } } */
+/* { dg-options "-O2 -fno-dse -fno-forward-propagate -Wno-psabi -mno-sse2" } */
+
+#define SHL(x, y) ((x) << ((y) & (sizeof(x) * 8 - 1)))
+#define SHR(x, y) ((x) >> ((y) & (sizeof(x) * 8 - 1)))
+#define ROR(x, y) (SHR(x, y)) | (SHL(x, (sizeof(x) * 8 - (y))))
+#define SHLV(x, y) ((x) << ((y) & (sizeof((x)[0]) * 8 - 1)))
+#define SHLSV(x, y) ((x) << ((y) & (sizeof((y)[0]) * 8 - 1)))
+typedef unsigned char A;
+typedef unsigned char __attribute__((__vector_size__ (8))) B;
+typedef unsigned char __attribute__((__vector_size__ (16))) C;
+typedef unsigned char __attribute__((__vector_size__ (32))) D;
+typedef unsigned char __attribute__((__vector_size__ (64))) E;
+typedef unsigned short F;
+typedef unsigned short __attribute__((__vector_size__ (16))) G;
+typedef unsigned int H;
+typedef unsigned int __attribute__((__vector_size__ (32))) I;
+typedef unsigned long long J;
+typedef unsigned long long __attribute__((__vector_size__ (8))) K;
+typedef unsigned long long __attribute__((__vector_size__ (32))) L;
+typedef unsigned long long __attribute__((__vector_size__ (64))) M;
+typedef unsigned __int128 N;
+typedef unsigned __int128 __attribute__((__vector_size__ (16))) O;
+typedef unsigned __int128 __attribute__((__vector_size__ (32))) P;
+typedef unsigned __int128 __attribute__((__vector_size__ (64))) Q;
+B v1;
+D v2;
+L v3;
+K v4;
+I v5;
+O v6;
+
+B
+foo (A a, C b, E c, F d, G e, H f, J g, M h, N i, P j, Q k)
+{
+ b &= (A) f;
+ k += a;
+ G l = e;
+ D m = v2 >= (A) (J) v1;
+ J r = a + g;
+ L n = v3 <= f;
+ k -= i / f;
+ l -= (A) g;
+ c |= (A) d;
+ b -= (A) i;
+ J o = ROR (__builtin_clz (r), a);
+ K p = v4 | f, q = v4 <= f;
+ P s = SHLV (SHLSV (__builtin_bswap64 (i), (P) (0 < j)) <= 0, j);
+ n += a <= r;
+ M t = (M) (a / SHLV (c, 0)) != __builtin_bswap64 (i);
+ I u = f - v5;
+ E v = (E) h + (E) t + (E) k;
+ D w = (union { D b[2]; }) { }.b[0] + ((union { E b; }) v).b[1] + m + (D) u + (D) n + (D) s;
+ C x = ((union { D b; }) w).b[1] + b + (C) l + (C) v6;
+ B y = ((union { C a; B b; }) x).b + ((union { C a; B b[2]; }) x).b[1] + (B) p + (B) q;
+ J z = i + o;
+ F z2 = z;
+ A z3 = z2;
+ return y + z3;
+}
+
+int
+main ()
+{
+ B x = foo (0, (C) { }, (E) { }, 10, (G) { }, 4, 2, (M) { }, 123842323652213865LL, (P) { 1 }, (Q) { });
+ if ((J) x != 0x2e2c2e2c2e2c2e30ULL)
+ __builtin_abort();
+ return 0;
+}
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2021-05-15 8:14 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-15 8:14 [gcc r12-808] regcprop: Fix another cprop_hardreg bug [PR100342] Jakub Jelinek
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).