| Message ID | 52D067EF.7020005@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 10, 2014 at 02:36:47PM -0700, Jeff Law wrote: > As mentioned in the PR. We have a memory load and two extensions. > > The first extension requires a copy because its source and > destination are not the same hard register. > > The second extension does not require a copy and has a wider mode > than the first extension. > > In this case we have to make sure to widen the copy we emit to > eliminate the first extension. Otherwise upper bits aren't going to > have their expected values. Thankfully the copy isn't emitted until > the end of ree and we have the modified memory reference to peek at > to get the proper mode. > > Bootstrapped and regression tested on x86_64-unknown-linux-gnu. OK > for the trunk? > --- a/gcc/ree.c > +++ b/gcc/ree.c > @@ -1003,11 +1003,20 @@ find_and_remove_re (void) > for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2) > { > rtx curr_insn = reinsn_copy_list[i]; > + rtx def_insn = reinsn_copy_list[i + 1]; > + > + /* Use the mode of the destination of the defining insn > + for the mode of the copy. This is necessary if the > + defining insn was used to eliminate a second extension > + that was wider than the first. */ > + rtx sub_rtx = *get_sub_rtx (def_insn); > rtx pat = PATTERN (curr_insn); > - rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)), > + rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), > REGNO (XEXP (SET_SRC (pat), 0))); > - rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat)); > - emit_insn_after (set, reinsn_copy_list[i + 1]); > + rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), > + REGNO (SET_DEST (pat))); > + rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src); > + emit_insn_after (set, def_insn); > } There is one thing I still worry about, if some target has an insn to say sign extend or zero extend a short memory load into HARD_REGNO_NREGS () > 1 register, but the other involved register has the only one (or fewer) hard registers available to it. Consider registers SImode hard registers 0, 1, 2, 3: (set (reg:SI 3) (something:SI)) (set (reg:HI 0) (expression:HI)) (set (reg:SI 2) (sign_extend:SI (reg:HI 0))) (set (reg:DI 0) (sign_extend:DI (reg:HI 0))) (use (reg:SI 3)) we transform this into: (set (reg:SI 3) (something:SI)) (set (reg:SI 2) (sign_extend:SI (expression:HI))) (set (reg:SI 0) (reg:HI 2)) (set (reg:DI 0) (sign_extend:DI (reg:HI 0))) (use (reg:SI 3)) first (well, the middle is then pending in copy list), and next: (set (reg:SI 3) (something)) (set (reg:DI 2) (sign_extend:DI (expression:HI))) (set (reg:DI 0) (reg:DI 2)) (use (reg:SI 3)) but that looks wrong, because the second instruction would now clobber (reg:SI 3). Dunno if we have such an target and thus if it is possible to construct a testcase. So, I'd say the handling of the second extend should notice that it is actually extending load into a different register and bail out if it would need more hard registers than it needed previously, or something similar. > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr59747.c > @@ -0,0 +1,28 @@ > +extern void abort (void) __attribute__ ((__noreturn__)); > +extern void exit (int) __attribute__ ((__noreturn__)); > +extern int printf (const char *, ...); The printf prototype isn't needed, and the noreturn attributes aren't needed either, as they are builtins, the compiler will add those automatically. Jakub
On 01/10/14 14:52, Jakub Jelinek wrote: > > There is one thing I still worry about, if some target has > an insn to say sign extend or zero extend a short memory load > into HARD_REGNO_NREGS () > 1 register, but the other involved > register has the only one (or fewer) hard registers available to it. > Consider registers SImode hard registers 0, 1, 2, 3: > (set (reg:SI 3) (something:SI)) > (set (reg:HI 0) (expression:HI)) > (set (reg:SI 2) (sign_extend:SI (reg:HI 0))) > (set (reg:DI 0) (sign_extend:DI (reg:HI 0))) > (use (reg:SI 3)) > we transform this into: > (set (reg:SI 3) (something:SI)) > (set (reg:SI 2) (sign_extend:SI (expression:HI))) > (set (reg:SI 0) (reg:HI 2)) > (set (reg:DI 0) (sign_extend:DI (reg:HI 0))) > (use (reg:SI 3)) > first (well, the middle is then pending in copy list), and next: > (set (reg:SI 3) (something)) > (set (reg:DI 2) (sign_extend:DI (expression:HI))) > (set (reg:DI 0) (reg:DI 2)) > (use (reg:SI 3)) > but that looks wrong, because the second instruction would now clobber > (reg:SI 3). Dunno if we have such an target and thus if it is possible > to construct a testcase. Seems like any port where we can do operations on 3 different modes and the largest mode requires > 1 hard reg ought to do. I don't see anything inherent in this setup that wouldn't apply to x86 for example. > > So, I'd say the handling of the second extend should notice that > it is actually extending load into a different register and bail out > if it would need more hard registers than it needed previously, or > something similar. That ought to be a trivial check to add. Let me add it and see if I can find something to trip it. > The printf prototype isn't needed, and the noreturn attributes > aren't needed either, as they are builtins, the compiler will > add those automatically. The printf prototype came from the original and I forgot to remove it when I zapped the printf statement. As for abort/exit, they're not strictly needed, but they shut up the compiler when running things by hand during testing/development. They're easy enough to zap. Thanks, Jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index a125517..4294831 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2014-01-10 Jeff Law <law@redhat.com> + + PR tree-optimization/59747 + * ree.c (find_and_remove_re): Properly handle case where a second + eliminated extension requires widening a copy created for elimination + of a prior extension. + 2014-01-09 Rong Xu <xur@google.com> * libgcc/libgcov-driver.c (this_prg): make it local to save diff --git a/gcc/ree.c b/gcc/ree.c index 1c4f3ad..0a34131 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -1003,11 +1003,20 @@ find_and_remove_re (void) for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2) { rtx curr_insn = reinsn_copy_list[i]; + rtx def_insn = reinsn_copy_list[i + 1]; + + /* Use the mode of the destination of the defining insn + for the mode of the copy. This is necessary if the + defining insn was used to eliminate a second extension + that was wider than the first. */ + rtx sub_rtx = *get_sub_rtx (def_insn); rtx pat = PATTERN (curr_insn); - rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)), + rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), REGNO (XEXP (SET_SRC (pat), 0))); - rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat)); - emit_insn_after (set, reinsn_copy_list[i + 1]); + rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), + REGNO (SET_DEST (pat))); + rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src); + emit_insn_after (set, def_insn); } /* Delete all useless extensions here in one sweep. */ diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index acb1637..1023133 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2014-01-10 Jeff Law <law@redhat.com> + + PR tree-optimization/59747 + * gcc.c-torture/execute/pr59747.c: New test. + 2014-10-09 Jakub Jelinek <jakub@redhat.com> PR sanitizer/59136 diff --git a/gcc/testsuite/gcc.c-torture/execute/pr59747.c b/gcc/testsuite/gcc.c-torture/execute/pr59747.c new file mode 100644 index 0000000..edb1685 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr59747.c @@ -0,0 +1,28 @@ +extern void abort (void) __attribute__ ((__noreturn__)); +extern void exit (int) __attribute__ ((__noreturn__)); +extern int printf (const char *, ...); + +int a[6], b, c = 1, d; +short e; + +int __attribute__ ((noinline)) +fn1 (int p) +{ + b = a[p]; +} + +int +main () +{ + if (sizeof (long long) != 8) + exit (0); + + a[0] = 1; + if (c) + e--; + d = e; + long long f = e; + if (fn1 ((f >> 56) & 1) != 0) + abort (); + exit (0); +}
As mentioned in the PR. We have a memory load and two extensions. The first extension requires a copy because its source and destination are not the same hard register. The second extension does not require a copy and has a wider mode than the first extension. In this case we have to make sure to widen the copy we emit to eliminate the first extension. Otherwise upper bits aren't going to have their expected values. Thankfully the copy isn't emitted until the end of ree and we have the modified memory reference to peek at to get the proper mode. Bootstrapped and regression tested on x86_64-unknown-linux-gnu. OK for the trunk? commit 7a83f984205b101f61dbfcabef59e8f459950f88 Author: Jeff Law <law@redhat.com> Date: Fri Jan 10 14:31:15 2014 -0700 PR tree-optimization/59747 * ree.c (find_and_remove_re): Properly handle case where a second eliminated extension requires widening a copy created for elimination of a prior extension. PR tree-optimization/59747 * gcc.c-torture/execute/pr59747.c: New test.