Exit MachineVerifier::verifyLiveRangeSegment prematurely if the current register being checked is physical and its root regunit reserved.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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.
Comment Actions
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 | This makes no sense: MCRegUnitRootIterator takes a register unit argument, you are passing a (plain) register instead. | |
Comment Actions
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?
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
Ping.
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.
Comment Actions
Matt, Matthias, could you please review the last version of the patch again? Stefan's comments look reasonable to me.
| lib/CodeGen/RegisterCoalescer.cpp | ||
|---|---|---|
| 1881–1882 ↗ | (On Diff #125715) | This is the same as LIS::removeAllRegUnitsForPhysReg I recently added. Can you assert all the regunits here are reserved? |
| lib/CodeGen/RegisterCoalescer.cpp | ||
|---|---|---|
| 1881–1882 ↗ | (On Diff #125715) | 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 makes no sense: MCRegUnitRootIterator takes a register unit argument, you are passing a (plain) register instead.