MachineVerifier tried to checkLivenessAtDef() ignoring it is actually a subreg.
The issue was with processing two subregs of the same reg are used in the same
instruction (e.g. inline asm): "def early-clobber" and other just "def".
Differential D126661
[MachineVerifier] Fix crash on early clobbered subreg operands. dfukalov on May 30 2022, 8:41 AM. Authored by
Details MachineVerifier tried to checkLivenessAtDef() ignoring it is actually a subreg. The issue was with processing two subregs of the same reg are used in the same
Diff Detail
Event TimelineComment Actions Beautified test.
Comment Actions I think this needs more explanation. It seems like it only fails verification when the same instruction has both a normal subreg def and an early-clobber subreg def of the same register. Is that correct? And the problem is that the super range starts at the early clobber slot (16e), but the subrange for the normal subreg def starts at the reg slot (16r): %0 [16e,32r:0) 0@16e L0000000000000003 [16e,32r:0) 0@16e L000000000000000C [16r,32r:0) 0@16r weight:0.000000e+00 ... *** Bad machine code: Inconsistent valno->def *** - function: sub0 - basic block: %bb.0 (0xbdc57a8) [0B;64B) - instruction: 16B INLINEASM &"" [attdialect], $0:[regdef-ec:VGPR_32], def undef early-clobber %0.sub0:vreg_64, $1:[regdef:VGPR_32], def undef %0.sub1:vreg_64 - operand 5: undef %0.sub1:vreg_64 - liverange: [16e,32r:0) 0@16e - v. register: %0 - ValNo: 0 (def 16e) - at: 16r I think your fix looks OK but I'd also like to hear opinions from @MatzeB or @qcolombet.
Comment Actions Jay, you are completely correct. Sorry I didn't mention the same info as in dependent review. Updated the description.
Comment Actions Ping - the patch with D127136 are fixing https://github.com/RadeonOpenCompute/ROCm/issues/1486... Comment Actions Sorry, but at a first glance this looks like you are just disabling verifier checks. The summary says that checkLivenessAtDef() would ignore that something is a subreg definition, but there are SubRangeCheck parameters and a LaneMask parameter, so this doesn't seem to be true in general. Is this really an incorrect check? Or does LLVM compute wrong liveness information? Could you share a dump of the computed liveintervals for the given example here? (I don't have an amdgpu-enabled build ready at the moment).
Comment Actions I guess Jay already posted the liveness info. It seems this is related to CalcLiveRangeUtilBase::createDeadDef moving definitions to the early clobber slot when there's an early-clobber and a normal def. Judging by the longer explanation in that function this is probably deliberate and we have to blame the MachineVerifier here. But that means you should fix the machine verifier and not just disable a whole subset of tests! I think to adapt to this, when you see an operand defined at a normal position but the live range starting at an early-clobber spot you need to do a 2nd pass making sure that there is another early-clobber def and in that case skip the warning! Comment Actions The actual issue here is we are calling checkLivenessAtDef(MO, MONum, DefIdx, *LI, Reg) where DefIdx is the slot for the checked operand that is subreg but LI is the interval for the whole superreg.
So if you mean that such usage of checkLivenessAtDef() is correct I can modify the function to skip warning.
Comment Actions Addresses comments. @foad please review I correctly implemented @MatzeB I double-checked that early-clobber def of superreg has a corresponding %0 [16e,32r:0) 0@16r L..3 [16r,32r:0) 0@16r L..C [16r,32r:0) 0@16r weight:0.000000e+00 generate *** Bad machine code: Live segment must begin at MBB entry or valno def *** %0 [16e,32r:0) 0@16e L..3 [16r,32r:0) 0@16r L..C [16r,32r:0) 0@16r weight:0.000000e+00 both of them are called from MachineVerifier::visitMachineFunctionAfter() (added mention to the comment) Comment Actions Addressed comment.
|
Don't need the last set of parens on this line.