public inbox for gcc-cvs@sourceware.org help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@gcc.gnu.org> To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-808] regcprop: Fix another cprop_hardreg bug [PR100342] Date: Sat, 15 May 2021 08:14:35 +0000 (GMT) [thread overview] Message-ID: <20210515081435.5A594385E036@sourceware.org> (raw) 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; +}
reply other threads:[~2021-05-15 8:14 UTC|newest] Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210515081435.5A594385E036@sourceware.org \ --to=jakub@gcc.gnu.org \ --cc=gcc-cvs@gcc.gnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).