Patch to fix unintentional(?) pessimisation in dbr_schedule
law@redhat.com
law@redhat.com
Fri Apr 9 20:06:00 GMT 2004
In message <20040401222758.GC31591@redhat.com>, Richard Henderson writes:
>On Sun, Mar 28, 2004 at 11:32:25AM +0100, Richard Sandiford wrote:
>> Note the indicated line. It once used AND_HARD_REG_SET, thus taking the
>> intersection of the two estimates, as described above. However, it was
>> changed to IOR_HARD_REG_SET by:
>>
>> http://gcc.gnu.org/ml/gcc-patches/1999-11n/msg00017.html
>>
>> So this code, which as far as I can tell was supposed to be an optimisation
>,
>> now only has the effect of _expanding_ the original estimate.
>
>Given that the referenced patch purported to fix a bug, and consisted
>*only* of this change, it doesn't seem safe to undo it without proving
>that the original bug can be fixed some other way.
>
>Unfortunately Diego did a poor job of explaining the situation, and
>gave no pointer to the original test case. Perhaps you can bug him
>to go back through old mail and figure out what this might have been.
Fun fun. You'd have to dig into the old Clarify database to find the
bug :-) CR 102182-CR-1 I have no idea if we can still get to that
data.
Here's the relevant parts of my message to Diego on the subject:
> /* If we hit an unconditional branch, we have another way of finding out
> what is live: we can see what is live at the branch target and include
> anything used but not set before the branch. The only things that are
> live are those that are live using the above test and the test below.
> */
You might consider changing the comment. I believe the last sentence is
rather ambiguous. In this case I believe "above test and the test below" is
supposed to mean the union, not the intersection. Any other interpretation
seems wrong to me.
[ ... ]
We want those things which were used before they were set as we approached
the jump plus anything that's live at the jump target.
> If I got the gist of this right, the resources referenced by the
> jump_target and the insns between target and stop_insn should be
> added to the resource information accumulated so far (res->regs).
> Therefore, you need the union of both sets, not their
> intersection. This seems to go against what the comments in the
> code say, however. So I might well be off target.
I believe you are correct.
> BTW, this bug is still in 99r1 and devo. It just so happened that
> by mere chance 99r1 is choosing different registers and so it's
> not triggering the problem.
Which would explain why none of the changes we scanned in the ChangeLog
looked all that promising as fixes for this problem.
> If this all made some sense, the following patch fixes the
> problem. I verified that it fixes this specific CR and I'm now
> doing regression tests.
I can't see how it could ever cause a regression since we're expanding the
set of things we consider live which is the safe thing to do.
jeff
More information about the Gcc-patches
mailing list