Message ID | orvanoryz2.fsf@lxoliva.fsfla.org |
---|---|
State | New |
Headers | show |
On Jun 22, 2017, Alexandre Oliva <aoliva@redhat.com> wrote: > The patch below (is this what you meant?) fixes the PR testcase, and the > new else block doesn't get exercised in an x86_64-linux-gnu bootstrap. Err, I misdescribed the situation. It's not that it doesn't get exercised, it's that this new block doesn't make any noticeable difference in the generated code. I checked that by adding a new option in common.opt, running the new block conditionally on the new option, building stage1 host normally, then continuing bootstrap with GCC_COMPARE_DEBUG=-fthe-new-option set in the environment, building target libs and stage2 build and host like that. GCC_COMPARE_DEBUG, like -fcompare-debug, checks that the compiler dumps at final are the same.
On Thu, Jun 22, 2017 at 09:25:21AM -0300, Alexandre Oliva wrote: > On Jun 8, 2017, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > Would it work to just have "else" instead if this "if"? Or hrm, we'll > > need to kill the recorded reg_stat value in the last case before this > > as well? > > The patch below (is this what you meant?) Yes exactly. > fixes the PR testcase, and the > new else block doesn't get exercised in an x86_64-linux-gnu bootstrap. > However, it seems to me that it might very well drop parallel SETs > without decrementing the sets count, though probably in cases in which > this won't matter. > > How's this? (I haven't run regression tests yet) Looks a lot better digestable than the previous patch, thanks! Things should probably be restructured a bit so we keep the sets count correct, if that is possible? > When an insn used by combine has multiple SETs, only the non-REG_UNUSED > set is used: others will end up dropped on the floor. This isn't exactly true, as I described in a previous email... You can write something simpler like If combine drops a REG_UNUSED SET, [...] > We have to take > note of the dropped REG_UNUSED SETs, clearing their cached values, so > that, even if the REGs remain used (e.g. because they were referenced > in the used SET_SRC), we will not use properties of the latest value > as if they applied to the earlier one. > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -14087,6 +14087,25 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, > PUT_REG_NOTE_KIND (note, REG_DEAD); > place = i3; > } > + > + /* If there were any parallel sets in FROM_INSN other than > + the one setting IDEST, it must be REG_UNUSED, otherwise > + we could not have used FROM_INSN in combine. Since this > + combine attempt succeeded, we know this unused SET was > + dropped on the floor, because the insn was either deleted > + or created from a new pattern that does not use its > + SET_DEST. We must forget whatever we knew about the > + value that was stored by that SET, since the prior value > + may still be present in IDEST's src expression or > + elsewhere, and we do not want to use properties of the > + dropped value as if they applied to the prior one when > + simplifying e.g. subsequent combine attempts. */ Similar for this comment. (It has a stray tab btw, before "We"). > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr80693.c > @@ -0,0 +1,26 @@ > +/* { dg-do run } */ > +/* { dg-options "-O -fno-tree-coalesce-vars" } */ > +typedef unsigned char u8; > +typedef unsigned short u16; > +typedef unsigned u32; > +typedef unsigned long u64; Maybe use "long long"? Less incorrect/misleading on 32-bit targets ;-) Thanks, Segher
diff --git a/gcc/combine.c b/gcc/combine.c index 2d49bc2..477010d 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14087,6 +14087,25 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, PUT_REG_NOTE_KIND (note, REG_DEAD); place = i3; } + + /* If there were any parallel sets in FROM_INSN other than + the one setting IDEST, it must be REG_UNUSED, otherwise + we could not have used FROM_INSN in combine. Since this + combine attempt succeeded, we know this unused SET was + dropped on the floor, because the insn was either deleted + or created from a new pattern that does not use its + SET_DEST. We must forget whatever we knew about the + value that was stored by that SET, since the prior value + may still be present in IDEST's src expression or + elsewhere, and we do not want to use properties of the + dropped value as if they applied to the prior one when + simplifying e.g. subsequent combine attempts. */ + else + { + gcc_assert (REG_P (XEXP (note, 0))); + record_value_for_reg (XEXP (note, 0), NULL, NULL_RTX); + INC_REG_N_SETS (REGNO (XEXP (note, 0)), -1); + } break; case REG_EQUAL: diff --git a/gcc/testsuite/gcc.dg/pr80693.c b/gcc/testsuite/gcc.dg/pr80693.c new file mode 100644 index 0000000..aecddd0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr80693.c @@ -0,0 +1,26 @@ +/* { dg-do run } */ +/* { dg-options "-O -fno-tree-coalesce-vars" } */ +typedef unsigned char u8; +typedef unsigned short u16; +typedef unsigned u32; +typedef unsigned long u64; + +static u64 __attribute__((noinline, noclone)) +foo(u8 u8_0, u16 u16_0, u32 u32_0, u64 u64_0, u16 u16_1) +{ + u16_1 += 0x1051; + u16_1 &= 1; + u8_0 <<= u32_0 & 7; + u16_0 -= !u16_1; + u16_1 >>= ((u16)-u8_0 != 0xff); + return u8_0 + u16_0 + u64_0 + u16_1; +} + +int +main (void) +{ + u64 x = foo(1, 1, 0xffff, 0, 1); + if (x != 0x80) + __builtin_abort(); + return 0; +}