Message ID | 52D84F9D.8050503@redhat.com |
---|---|
State | New |
Headers | show |
On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote: > +2014-01-16 Jeff Law <law@redhat.com> > + > + * ree.c (combine_set_extension): Correct test for changing number > + of hard registers when widening a reaching definition. > + > 2014-01-16 Bernd Schmidt <bernds@codesourcery.com> > > PR middle-end/56791 > diff --git a/gcc/ree.c b/gcc/ree.c > index 19d821c..96cddd2 100644 > --- a/gcc/ree.c > +++ b/gcc/ree.c > @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set) > /* We're going to be widening the result of DEF_INSN, ensure that doing so > doesn't change the number of hard registers needed for the result. */ > if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) > - != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set)))) > + != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)), > + GET_MODE (SET_DEST (*orig_set)))) Shouldn't that be: if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) != HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set)))) instead? I mean, for the !copy_needed case it is obviously the same thing (and that is what triggers in the testcase), but don't we generally want to check if the same hard register in a wider mode will not occupy more registers, and in particular the hard register we are considering to use on the lhs of the defining insn (i.e. new_reg)? Of course usually HARD_REGNO_NREGS will be the same for the same mode and different GPR, so it would be really hard to create a testcase. Jakub
> Bootstrapped & regression tested on x86_64-unknown-linux. Also > bootstrapped with --enable-checking=rtl. Note that you can do only one bootstrap with --enable-checking=yes,rtl. > Installed on the trunk. As far as I can see, no, it was not installed.
On 01/17/14 01:41, Eric Botcazou wrote: >> Bootstrapped & regression tested on x86_64-unknown-linux. Also >> bootstrapped with --enable-checking=rtl. > > Note that you can do only one bootstrap with --enable-checking=yes,rtl. > >> Installed on the trunk. > > As far as I can see, no, it was not installed. Pilot error, it's not installed. jefff
On 01/16/14 15:07, Jakub Jelinek wrote: > On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote: >> +2014-01-16 Jeff Law <law@redhat.com> >> + >> + * ree.c (combine_set_extension): Correct test for changing number >> + of hard registers when widening a reaching definition. >> + >> 2014-01-16 Bernd Schmidt <bernds@codesourcery.com> >> >> PR middle-end/56791 >> diff --git a/gcc/ree.c b/gcc/ree.c >> index 19d821c..96cddd2 100644 >> --- a/gcc/ree.c >> +++ b/gcc/ree.c >> @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set) >> /* We're going to be widening the result of DEF_INSN, ensure that doing so >> doesn't change the number of hard registers needed for the result. */ >> if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) >> - != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set)))) >> + != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)), >> + GET_MODE (SET_DEST (*orig_set)))) > > Shouldn't that be: > if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) > != HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set)))) > instead? > > I mean, for the !copy_needed case it is obviously the same thing (and that > is what triggers in the testcase), but don't we generally want to check if > the same hard register in a wider mode will not occupy more registers, and > in particular the hard register we are considering to use on the lhs of the > defining insn (i.e. new_reg)? I thought about using that conditional more than once. But talked myself out of it every time on the grounds that I wanted to test the original destination REGNO of the reaching def. Obviously that is REGNO (new_reg) if !copy_needed. But it's something completely different if copy_needed. In the copy_needed case there's actually two destinations to consider. The original destination as well as the new destination. Both will be set in a mode wider than the destination of the original reaching def. (one will be set in the modified reaching def and the other in a copy insn). ISTM we need the # hard reg checked on the original destination as the other (upper) hard regs might be live across the sequence, but not used/set in the sequence. Then we need some kind of check on the upper part of the new destination... But I thought I covered that elsewhere... Anyway, I clearly need to rethink that test. Given this is something we haven't seen in the wild, I'm going to disable it over the weekend/monday so that enable-checking bugs pass and continue to ponder. jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index e07d1ae..3fa6f5f 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,8 @@ +2014-01-16 Jeff Law <law@redhat.com> + + * ree.c (combine_set_extension): Correct test for changing number + of hard registers when widening a reaching definition. + 2014-01-16 Bernd Schmidt <bernds@codesourcery.com> PR middle-end/56791 diff --git a/gcc/ree.c b/gcc/ree.c index 19d821c..96cddd2 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set) /* We're going to be widening the result of DEF_INSN, ensure that doing so doesn't change the number of hard registers needed for the result. */ if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) - != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set)))) + != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)), + GET_MODE (SET_DEST (*orig_set)))) return false; /* Merge constants by directly moving the constant into the register under