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