| Message ID | CAFULd4ZEJa7XFOxOa86+rN202QqyBa6D6k-vwPBxaf24Ek1XAw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> The solution is to fix the scanning loop to look into the insn pattern > itself for all set and clobbered hard registers. This way, all > registers, clobbered by the pattern, will be correctly marked in the > "live" bitmap, including FLAGS register that is ignored by current > approach. > > 2012-01-12 Uros Bizjak <ubizjak@gmail.com> > > * recog.c (peep2_find_free_register): Determine clobbered registers > from insn pattern. Is that a complete solution though? Don't we need to do more, for example because of peep2_reg_dead_p and peep2_update_life? These are not rhetorical questions, but genuine ones; it's a little disturbing to discover such a flaw in this kind of code after all these years (I can reproduce the problem with all the compilers of the 4.x series, so this didn't work with flow.c either). Thanks for debugging this in any case.
On Fri, Jan 13, 2012 at 12:01 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> The solution is to fix the scanning loop to look into the insn pattern >> itself for all set and clobbered hard registers. This way, all >> registers, clobbered by the pattern, will be correctly marked in the >> "live" bitmap, including FLAGS register that is ignored by current >> approach. >> >> 2012-01-12 Uros Bizjak <ubizjak@gmail.com> >> >> * recog.c (peep2_find_free_register): Determine clobbered registers >> from insn pattern. > > Is that a complete solution though? Don't we need to do more, for example > because of peep2_reg_dead_p and peep2_update_life? These are not rhetorical > questions, but genuine ones; it's a little disturbing to discover such a flaw > in this kind of code after all these years (I can reproduce the problem with > all the compilers of the 4.x series, so this didn't work with flow.c either). Yes, it is a complete solution. Tracking register liveness is different issue, and a register is indeed dead after instruction, if it has been clobbered by insn, or when marked unused. This works OK, and there are many examples of peep2_{reg|regno}_dead_p usag in i386.md. The problem my patch solves is the answer to the question "Is the choosen non-live temporary register untouched over the insn sequence?". The answer: "Yes, if it was not set or clobbered by any insn in the sequence". Uros.
> Yes, it is a complete solution. Tracking register liveness is > different issue, and a register is indeed dead after instruction, if > it has been clobbered by insn, or when marked unused. My concern was liveness within the new sequence of instructions: suppose you have (set (reg:M x) (...)) REG_UNUSED (reg:M x) and we "peephole" the instruction. Is the live range of (reg:M x) properly extended within the new sequence of instructions? > The problem my patch solves is the answer to the question "Is the > choosen non-live temporary register untouched over the insn > sequence?". The answer: "Yes, if it was not set or clobbered by any > insn in the sequence". Are you sure that you don't need to do this in addition to the existing test, instead of in lieu of the existing test?
On Fri, Jan 13, 2012 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Yes, it is a complete solution. Tracking register liveness is >> different issue, and a register is indeed dead after instruction, if >> it has been clobbered by insn, or when marked unused. > > My concern was liveness within the new sequence of instructions: suppose you > have > > (set (reg:M x) (...)) REG_UNUSED (reg:M x) > > and we "peephole" the instruction. Is the live range of (reg:M x) properly > extended within the new sequence of instructions? The patch doesn't change this functionality, but: (match_scratch:M 1 "r") (set (reg:M x) (...)) REG_UNUSED (reg:M x) (match_dup 1) Previously, register x would be allocated as a scracth register, but now it isn't. Regarding the liveness analysis of a new sequence - this functionality is not changed at all. As said, the new code only prevents x to be allocated as a scratch that must live up to the (match_dup 1). >> The problem my patch solves is the answer to the question "Is the >> choosen non-live temporary register untouched over the insn >> sequence?". The answer: "Yes, if it was not set or clobbered by any >> insn in the sequence". > > Are you sure that you don't need to do this in addition to the existing test, > instead of in lieu of the existing test? Yes, because new test ALWAYS includes the registers that were wrongly marked as dead by previous test due to REG_UNUSED and noclobber processing. Uros.
Uros Bizjak <ubizjak@gmail.com> writes: > On Fri, Jan 13, 2012 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> The problem my patch solves is the answer to the question "Is the >>> choosen non-live temporary register untouched over the insn >>> sequence?". The answer: "Yes, if it was not set or clobbered by any >>> insn in the sequence". >> >> Are you sure that you don't need to do this in addition to the existing test, >> instead of in lieu of the existing test? > > Yes, because new test ALWAYS includes the registers that were wrongly > marked as dead by previous test due to REG_UNUSED and noclobber > processing. Yeah, the patch looks like the right fix to me FWIW. Being able to use DF_INSN_DEFS in this way is part of the point of having df: when going from one insn to the next within the same bb, there are no "hidden" influences that would cause a register to become live outside of DF_INSN_DEFS. The only requirement is that we call df_insn_rescan on new instructions, and we do (in peep2_update_life, where we need it for df_simulate_one_insn_backwards). Or to put it another way: the insn-to-insn changes in the current liveness sets are all produced by df_simulate_one_insn_{backwards,forwards}, which uses the same information. So I don't think there's any reason why we need to keep the current code as well. Richard
On 01/13/2012 10:58 AM, Richard Sandiford wrote: > > Yes, because new test ALWAYS includes the registers that were wrongly > > marked as dead by previous test due to REG_UNUSED and noclobber > > processing. > > Or to put it another way: the insn-to-insn changes in the current liveness > sets are all produced by df_simulate_one_insn_{backwards,forwards}, which > uses the same information. So I don't think there's any reason why we > need to keep the current code as well. Yes, both explanations are sound. I think the patch is good, too. Paolo
On Fri, Jan 13, 2012 at 10:58 AM, Richard Sandiford <richard.sandiford@linaro.org> wrote: >>>> The problem my patch solves is the answer to the question "Is the >>>> choosen non-live temporary register untouched over the insn >>>> sequence?". The answer: "Yes, if it was not set or clobbered by any >>>> insn in the sequence". >>> >>> Are you sure that you don't need to do this in addition to the existing test, >>> instead of in lieu of the existing test? >> >> Yes, because new test ALWAYS includes the registers that were wrongly >> marked as dead by previous test due to REG_UNUSED and noclobber >> processing. > > Yeah, the patch looks like the right fix to me FWIW. Being able to use > DF_INSN_DEFS in this way is part of the point of having df: when going > from one insn to the next within the same bb, there are no "hidden" > influences that would cause a register to become live outside of > DF_INSN_DEFS. The only requirement is that we call df_insn_rescan > on new instructions, and we do (in peep2_update_life, where we need > it for df_simulate_one_insn_backwards). > > Or to put it another way: the insn-to-insn changes in the current liveness > sets are all produced by df_simulate_one_insn_{backwards,forwards}, which > uses the same information. So I don't think there's any reason why we > need to keep the current code as well. Patch was approved offline by Richard Sandiford, so committed to mainline SVN, will be committed to release branches after bootstrap & regression tests. Thanks, Uros.
Index: recog.c =================================================================== --- recog.c (revision 183130) +++ recog.c (working copy) @@ -3038,6 +3038,7 @@ peep2_find_free_register (int from, int to, const static int search_ofs; enum reg_class cl; HARD_REG_SET live; + df_ref *def_rec; int i; gcc_assert (from < MAX_INSNS_PER_PEEP2 + 1); @@ -3051,12 +3052,14 @@ peep2_find_free_register (int from, int to, const while (from != to) { - HARD_REG_SET this_live; + gcc_assert (peep2_insn_data[from].insn != NULL_RTX); + /* Don't use registers set or clobbered by the insn. */ + for (def_rec = DF_INSN_DEFS (peep2_insn_data[from].insn); + *def_rec; def_rec++) + SET_HARD_REG_BIT (live, DF_REF_REGNO (*def_rec)); + from = peep2_buf_position (from + 1); - gcc_assert (peep2_insn_data[from].insn != NULL_RTX); - REG_SET_TO_HARD_REG_SET (this_live, peep2_insn_data[from].live_before); - IOR_HARD_REG_SET (live, this_live); } cl = (class_str[0] == 'r' ? GENERAL_REGS Index: testsuite/gcc.dg/pr51821.c =================================================================== --- testsuite/gcc.dg/pr51821.c (revision 0) +++ testsuite/gcc.dg/pr51821.c (revision 0) @@ -0,0 +1,24 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -msse" { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-require-effective-target sse_runtime { target { i?86-*-* x86_64-*-* } } } */ + +extern void abort (void); + +unsigned int __attribute__((noinline)) +test (int shift_size) +{ + unsigned long long res = ~0; + + return res << shift_size; +} + +int +main () +{ + int dst = 32; + + if (test (dst) != 0) + abort (); + + return 0; +}
Hello! As discussed in the PR, attached testcase uncovers problem with the way peephole2 pas determines free registers to allocate temporary register. The problem was with the pattern such as: (insn 7 19 16 2 (parallel [ (set (reg:DI 0 ax [65]) (ashift:DI (const_int -1 [0xffffffffffffffff]) (reg:QI 2 cx [orig:63 shift_size ] [63]))) (clobber (reg:CC 17 flags)) ]) pr51821.c:8 489 {*ashldi3_doubleword} (expr_list:REG_DEAD (reg:QI 2 cx [orig:63 shift_size ] [63]) (expr_list:REG_UNUSED (reg:CC 17 flags) (expr_list:REG_UNUSED (reg:SI 1 dx) (expr_list:REG_EQUAL (ashift:DI (const_int -1 [0xffffffffffffffff]) (subreg:QI (reg/v:SI 2 cx [orig:63 shift_size ] [63]) 0)) (nil)))))) where DImode ax/dx register pair gets defined, while at the same time, one of the registers from the register pair gets marked as unused. Currently, peephole2 pass determines clobbered registers from DF life analysis, as the difference of registers, live before the insn sequence and after insn sequence. Unfortunately, as shown by attached testcase in the PR, this approach is not correct. In the above pattern, dx is considered "dead" due to (REG_UNUSED dx) note. The solution is to fix the scanning loop to look into the insn pattern itself for all set and clobbered hard registers. This way, all registers, clobbered by the pattern, will be correctly marked in the "live" bitmap, including FLAGS register that is ignored by current approach. 2012-01-12 Uros Bizjak <ubizjak@gmail.com> * recog.c (peep2_find_free_register): Determine clobbered registers from insn pattern. testsuite/ChangeLog: 2012-01-12 Uros Bizjak <ubizjak@gmail.com> * gcc.dg/pr51821.c: New test. Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu. OK for mainline and release branches? Uros.