Fix out-of-bounds BitVector access in RegScavenger

[bmoody]

Summary

RegScavenger::determineKillsAndDefs was calling BitVector::clear() which
sets the size of the BitVector to 0. Changed to reset() which un-sets the
contents of the vector but doesn't change the size.

This code started tripping an assert after a change I'm working on in BitVector.
I haven't included a test because I have no idea how to exercise this code.

Not sure who to send this review to, looks like this bug was introduced in 2014 and doesn't get much attention.

[bmoody]

Ping! :) Would anyone be able to take a look at this? This is blocking another change I want to make in BitVector causes this code to assert out instead of silently misbehaving.

[efriedma]

This code started tripping an assert after a change I'm working on in BitVector.
I haven't included a test because I have no idea how to exercise this code.

Those two sentences seem like a contradiction; clearly you did figure out how to exercise the code, if you triggered an assertion. :)

[bmoody]

When I add the assert to BitVector::set, 40 codegen tests start failing but those tests are currently passing so they're not really exercising this behaviour. It's easy enough to write a test that executes this code, but I don't know how I'd write a test that fails due to this bug.

[efriedma]

Oh, I see what you mean; if you assert that the access is in bounds, it asserts, but otherwise you can't show it actually misbehaves.

Really, there are only three operations involving TmpRegUnits; clear(), set(), and operator|=(). The sum result, I think, is that the operator|=() does nothing at all.

Given the code is currently "dead", this change is essentially adding new functionality. I don't want to do that without appropriate tests.

[arsenm]

llvm/lib/CodeGen/RegisterScavenging.cpp

126

I assume if you put an unreachable here, something breaks? If you find that testcase, it should be easy to derive one

[bmoody]

In D77029#2107348, @efriedma wrote:

Given the code is currently "dead", this change is essentially adding new functionality. I don't want to do that without appropriate tests.

Very understandable. I've spent some more time trying to derive a codegen test for this change but I'm having a hard time as I don't have much experience with the codegen side of llvm. Could you recommend someone with knowledge of this code who might be able to help me?

[arsenm]

In D77029#2224981, @bmoody wrote:

In D77029#2107348, @efriedma wrote:

Given the code is currently "dead", this change is essentially adding new functionality. I don't want to do that without appropriate tests.

Very understandable. I've spent some more time trying to derive a codegen test for this change but I'm having a hard time as I don't have much experience with the codegen side of llvm. Could you recommend someone with knowledge of this code who might be able to help me?

Did you try adding the unreachable as I suggested above?

[efriedma]

Did you try adding the unreachable as I suggested above?

The code is reachable. The problem is coming up with a testcase where the change has a visible effect; in the vast majority of cases, it won't (because we don't use the resulting RegScavenger state, or the register is known dead anyway, or there's another free register).

[MatzeB]

Ugh... I suspect we're not seeing a problem in practice since missing "kills" just means missed opportunities but it's not a correctness problem.

If we don't want the risk of changing existing behavior then we could replace the whole loop with a TODO comment for now.

BTW: If you are changing the BitVector class anyway, would it be possible to remove the clear function (or change its behavior). Given that most users think of a BitVector more as an integer set (and not like a vector that you push_back() to) the behavior can confuse fast...

llvm/lib/CodeGen/RegisterScavenging.cpp

120–134

120–134

Given that this appears to do nothing, let's go for this:

[MatzeB]

llvm/lib/CodeGen/RegisterScavenging.cpp

120–134

(this should not include the for line, but just the if (MO.isRegMask()) part.

[bmoody]

Looks like someone else already changed the clear to a reset as part of a different change to BitVector. Commit 550ed575cbbd2c81cb855d4debb4a215ae75ef1e.

llvm/lib/CodeGen/RegisterScavenging.cpp

void RegScavenger::determineKillsAndDefs() {
 
   MachineInstr &MI = *MBBI;
   assert(!MI.isDebugInstr() && "Debug values have no kills or defs");
 
   // Find out which registers are early clobbered, killed, defined, and marked
   // def-dead in this instruction.
   KillRegUnits.reset();
   DefRegUnits.reset();
   for (const MachineOperand &MO : MI.operands()) {
     if (MO.isRegMask()) {
-      TmpRegUnits.clear();
+      TmpRegUnits.reset();
       for (unsigned RU = 0, RUEnd = TRI->getNumRegUnits(); RU != RUEnd; ++RU) {
         for (MCRegUnitRootIterator RURI(RU, TRI); RURI.isValid(); ++RURI) {
           if (MO.clobbersPhysReg(*RURI)) {
             TmpRegUnits.set(RU);
             break;
           }
         }
       }
 
       // Apply the mask.
       KillRegUnits |= TmpRegUnits;
     }
     if (!MO.isReg())
       continue;
     Register Reg = MO.getReg();
     if (!Register::isPhysicalRegister(Reg) || isReserved(Reg))
       continue;
 
     if (MO.isUse()) {
       // Ignore undef uses.