This is an archive of the discontinued LLVM Phabricator instance.

Skip live range segment verification for reserved physregs
AbandonedPublic

Authored by smaksimovic on Jul 28 2017, 3:29 AM.

Details

Summary

Exit MachineVerifier::verifyLiveRangeSegment prematurely if the current register being checked is physical and its root regunit reserved.

Diff Detail

Event Timeline

smaksimovic created this revision.Jul 28 2017, 3:29 AM

Test where this was spotted is test/CodeGen/Mips/no-odd-spreg-msa.ll which fails when checked with machine verifier on.
What exactly happens is that a value is moved from a physical register which is not reserved($f12) to a physical register which
in this case is($f13) via inline asm.

...
64B             INLINEASM <es:mov.s $0, $1> [sideeffect] [attdialect], $0:[regdef], %F13<imp-def>, $1:[reguse], %F12
80B             %vreg2<def> = COPY %F13; FGR32:%vreg2
96B             %vreg3<def> = LW %vreg1, <ga:@v4f32>[TF=1]; mem:LD4[GOT] GPR32:%vreg3,%vreg1
112B            %vreg4<def> = LD_W %vreg3, 0; mem:Volatile LD16[@v4f32](dereferenceable) MSA128W:%vreg4 GPR32:%vreg3
128B            %vreg8<def> = SUBREG_TO_REG 0, %vreg2, sub_lo; MSA128WEvens:%vreg8 FGR32:%vreg2
...

The reserved one is an operand of a COPY instruction which will later get evicted after register coalescing.

...
80B     %vreg2<def> = COPY %F13; FGR32:%vreg2
        Considering merging %vreg2 with %F13
                RHS = %vreg2 [80r,128r:0)  0@80r
                updated: 128B   %vreg8<def> = SUBREG_TO_REG 0, %F13, sub_lo; MSA128WEvens:%vreg8
        Success: %vreg2 -> %F13
        Result = %F13
...

After merging:

...
64B             INLINEASM <es:mov.s $0, $1> [sideeffect] [attdialect], $0:[regdef], %F13<imp-def>, $1:[reguse], %F12
96B             %vreg3<def> = LW %vreg1, <ga:@v4f32>[TF=1]; mem:LD4[GOT] GPR32:%vreg3,%vreg1
112B            %vreg5<def> = LD_W %vreg3, 0; mem:Volatile LD16[@v4f32](dereferenceable) MSA128W:%vreg5 GPR32:%vreg3
128B            %vreg8<def> = SUBREG_TO_REG 0, %F13, sub_lo; MSA128WEvens:%vreg8
...

The problem here is that the F13 regunit does not get its intervals updated and the error is emitted in MachineVerifier::verifyLiveRangeSegment
in the block which reports "Live segment doesn't end at a valid instruction" error message, due to non-existing instuction at live segment end.

F13 [64r,80r:0) 0@64r

I've searched for information on handling live ranges of physical registers, particularly reserved ones and found this:
http://lists.llvm.org/pipermail/llvm-dev/2016-February/095999.html
and came to the conclusion that reserved physical registers are assumed always live hence their live segments shouldn't be taken into consideration
since they aren't updated properly as can be seen from the output above.

MatzeB requested changes to this revision.Aug 4 2017, 4:28 PM

I would expect the live ranges of register units that are reserved to be empty and therefore naturally pass verification. Do you know why you end up having non-empty information in those live ranges? We should identify the cause for that and fix it.

lib/CodeGen/MachineVerifier.cpp
1788–1792 ↗(On Diff #108611)

This makes no sense: MCRegUnitRootIterator takes a register unit argument, you are passing a (plain) register instead.

This revision now requires changes to proceed.Aug 4 2017, 4:28 PM

Sorry for not getting back at this earlier.

I inquired for the live range information of the register unit belonging to the $f13 register at the beginning of RegisterCoalescer::runOnMachineFunction() and it turned out to be non-empty.
Checking for the same live range information in the Live Variable Analysis pass which directly precedes Simple Register Coalescing pass revealed that it is empty.
Since the passes are executed one after another I couldn't pinpoint where exactly the live range information is generated.

As a side note, we could remove the live range for the register unit in question at the end of RegisterCoalescer::joinReservedPhysReg() right before we delete the COPY machine instruction.
Would that solution be accceptable?

bjope added a subscriber: bjope.Dec 3 2017, 2:57 AM

I would expect the live ranges of register units that are reserved to be empty and therefore naturally pass verification. Do you know why you end up having non-empty information in those live ranges? We should identify the cause for that and fix it.

Not at all sure if this is involved in the problems seen here, but isn't this a fault (in case there should be no dead reserved registers). The code is from LivePhysRegs.cpp (and it is used by the IfConverter):

void llvm::recomputeLivenessFlags(MachineBasicBlock &MBB) {
  const MachineFunction &MF = *MBB.getParent();
  const MachineRegisterInfo &MRI = MF.getRegInfo();
  const TargetRegisterInfo &TRI = *MRI.getTargetRegisterInfo();

  // We walk through the block backwards and start with the live outs.
  LivePhysRegs LiveRegs;
  LiveRegs.init(TRI);
  LiveRegs.addLiveOutsNoPristines(MBB);

  for (MachineInstr &MI : make_range(MBB.rbegin(), MBB.rend())) {
    // Recompute dead flags.
    for (MIBundleOperands MO(MI); MO.isValid(); ++MO) {
      if (!MO->isReg() || !MO->isDef() || MO->isDebug())
        continue;

      unsigned Reg = MO->getReg();
      if (Reg == 0)
        continue;
      assert(TargetRegisterInfo::isPhysicalRegister(Reg));

      bool IsNotLive = LiveRegs.available(MRI, Reg);
      MO->setIsDead(IsNotLive);
    }

   ...

The code above will set "dead" on reserved register defs since LivePhysRegs::available returns false for reserved regs, according to:

bool LivePhysRegs::available(const MachineRegisterInfo &MRI,
                             unsigned Reg) const {
  if (LiveRegs.count(Reg))
    return false;
  if (MRI.isReserved(Reg))
    return false;
  for (MCRegAliasIterator R(Reg, TRI, false); R.isValid(); ++R) {
    if (LiveRegs.count(*R))
      return false;
  }
  return true;
}

PS. llvm::recomputeLivenessFlags is also setting "kill" on reserved register uses. I assume that is incorrect as well.

It seems that at some places mentioned above I have erroneously acquired for the register unit's live range. The method which I used was getRegUnit() which computes ranges if they are missing, which I have somehow overlooked.
Looking at RegisterCoalescer::joinReservedPhysReg(CoalescerPair &CP) in RegisterCoalescer.cpp there exists a statement

if (RHS.overlaps(LIS->getRegUnit(*UI)))

Checking inside this specific call it can be seen that the live range for this register unit is computed, i.e. it didn't exist up to this point.
By using LIS->getCachedRegUnit(*UI) instead and conditionally calling getRegUnit() in the following if statement I was able to get the test to pass, and found that no live range information was present for the register unit in question this time.

Would this method be preferable to the one I suggested above?

smaksimovic edited edge metadata.

Updated patch with my first suggestion by removing the live range of the register unit right before deleting the copy machine instruction as opposed to the attempt mentioned in my previous message.
Problem with the latter was that it seems we couldn't simply skip the interference check if the register unit didn't have its live range computed at that point, so we couldn't take that route.

Ping.

I would expect the live ranges of register units that are reserved to be empty and therefore naturally pass verification. Do you know why you end up having non-empty information in those live ranges? We should identify the cause for that and fix it.

The LiveRange for F13 reserved register unit is initially empty but gets computed on demand at
RHS.overlaps(LIS->getRegUnit(*UI))
when joining intervals (RegisterCoalescer.cpp, joinReservedPhysReg function).
This is where we get the live range populated for this register unit.

After the copy instruction which uses $f13 gets removed, the previously computed live range however stays.
Since the live range for said register unit was empty in the first place and was computed only for the interval joining operation,
does discarding it make sense prior to deleting the copy instruction?
I didn't notice any regressions in the codegen test suite.

atanasyan added a subscriber: atanasyan.

Matt, Matthias, could you please review the last version of the patch again? Stefan's comments look reasonable to me.

arsenm added inline comments.Mar 11 2019, 2:06 PM
lib/CodeGen/RegisterCoalescer.cpp
1881–1882

This is the same as LIS::removeAllRegUnitsForPhysReg I recently added.

Can you assert all the regunits here are reserved?

atanasyan added inline comments.Mar 11 2019, 3:51 PM
lib/CodeGen/RegisterCoalescer.cpp
1881–1882

Good point. Adding the assert(MRI->isReservedRegUnit(*UI) to the loop causes crashes. The last register unit in a list is not reserved. I need to investigate that.

This problem has been fixed at rL357472 and the no-odd-spreg-msa.ll test case passes with the -verify-machineinstrs option. D35985 can be closed / abandoned.

smaksimovic abandoned this revision.Apr 8 2019, 7:37 AM

Understood, glad to see it finally resolved.