Page MenuHomePhabricator

[MachineInstr] Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand
ClosedPublic

Authored by jackoalan on Sep 20 2021, 1:35 PM.

Details

Summary

Based on the reasoning of D53903, register operands of DBG_VALUE are invariably treated as RegState::Debug operands. This change enforces this invariant as part of MachineInstr::addOperand so that all passes emit this flag consistently.

RegState::Debug is inconsistently set on DBG_VALUE registers throughout LLVM. This runs the risk of a filtering iterator like MachineRegisterInfo::reg_nodbg_iterator to process these operands erroneously when not parsed from MIR sources.

This issue was observed in the development of the llvm-mos fork which adds a backend that relies on physical register operands much more than existing targets. Physical RegUnit 0 has the same numeric encoding as $noreg (indicating an undef for DBG_VALUE). Allowing debug operands into the machine scheduler correlates $noreg with RegUnit 0 (i.e. a collision of register numbers with different zero semantics). Eventually, this causes an assert where DBG_VALUE instructions are prohibited from participating in live register ranges.

Diff Detail

Event Timeline

jackoalan created this revision.Sep 20 2021, 1:35 PM
jackoalan requested review of this revision.Sep 20 2021, 1:35 PM
jackoalan retitled this revision from Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand to [MachineInstr] Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand.Sep 21 2021, 12:21 AM

This looks like a strict improvement, since the new location for calling setIsDebug will catch all the old cases and more. There is one gap though - MachineOperand->ChangeToRegister(0, false) can be called on an operand for a DBG_VALUE (and is in multiple places), which sounds like it could cause the same problem you've observed; plugging this gap may require adding a similar block to that function.

This could probably do with a test - isDebug isn't printed to MIR, and it sounds like the issue only appears on a backend that isn't in LLVM, so a lit test probably isn't feasible. A simple unit test could work though: possibly a test in MachineInstrTest that creates a MachineInstr, adds MachineOperands, and confirms that they're debug registers.

jackoalan updated this revision to Diff 375677.Sep 28 2021, 1:04 PM

Cover MachineOperand::ChangeToRegister with invariant. Add unit test to MachineInstrTest.cpp

Seems to me that the most important thing would be to add a check to MachineVerifier so we consistently catch problems. Adding automation to addOperand is fine, but there are enough ways to change instructions and operands with other APIs.

If you could find the time to add a check to MachineVerifier that would be fantastic! Though we can also leave that for another time, up to you.

Changes so far look fine, but if you add the automation, could you go through the sourcecode and remove manual usage of setIsDebug() then?

Shouldn't be too difficult to add the MachineVerifier check and remove the setIsDebug calls. Shall I also remove RegState::Debug in the context of DBG_VALUE builders?

jackoalan added a comment.EditedSep 28 2021, 3:45 PM

Also curious if invariant debug register operands are applicable to other debug instructions (DBG_INSTR_REF, DBG_PHI, DBG_LABEL)

jackoalan updated this revision to Diff 376710.Oct 2 2021, 10:23 AM

Add check to MachineVerifier. Remove explicit debug operand flag uses.

Very nice cleanup!

Also curious if invariant debug register operands are applicable to other debug instructions (DBG_INSTR_REF, DBG_PHI, DBG_LABEL)

I think that this change should probably include DBG_PHIs as well - you could probably just replace isDebugValue() with isDebugInstr(), as any current or future debug instructions that use registers should have them set isDebug.

jackoalan updated this revision to Diff 377360.Oct 5 2021, 3:05 PM

Apply to all debug instructions according to MachineInstr::isDebugInstr

MatzeB added a comment.Oct 5 2021, 5:20 PM

Thanks, great work cleaning all up all the use sites! I have some comments on the verifier changes, then this will be good to go.

llvm/lib/CodeGen/MachineVerifier.cpp
1894–1895
  • Use isDebugInstr() here as well and tweak message.
  • Also check the reverse: non-debug instructions must not use the debug flag.
  • Also test for isUse() for good measure (even though it should not be possible for debug instruction to ever define regs).
1966–1967

It seems a similar check already existed but was incorrectly only put into the isPhysicalRegister() branch... We can drop it now I guess.

jackoalan updated this revision to Diff 377401.Oct 5 2021, 5:39 PM

Remove now-redundant MachineVerifier check. Also check debug instructions on uses only and non-debug instruction case.

jackoalan marked 2 inline comments as done.Oct 5 2021, 5:40 PM
MatzeB accepted this revision.Oct 5 2021, 5:44 PM

LGTM

This revision is now accepted and ready to land.Oct 5 2021, 5:44 PM

Thank you for the feedback! Would you mind committing this? --author="Jack Andersen <jackoalan@gmail.com>"