I got "Use not jointly dominated by defs" when removePartialRedundancy
attempted to prune then re-extend a subrange whose only liveness was a
dead def at the copy being removed.
Change-Id: I6f894e9f517f71e921e0c6d81d28c5f344db8dad
Paths
| Differential D50914
[RegisterCoalescer] Fix for assert in removePartialRedundancy ClosedPublic Authored by tpr on Aug 17 2018, 11:50 AM.
Details Summary I got "Use not jointly dominated by defs" when removePartialRedundancy Change-Id: I6f894e9f517f71e921e0c6d81d28c5f344db8dad
Diff Detail
Event TimelineHerald added subscribers: llvm-commits, nhaehnle, qcolombet, MatzeB. · View Herald TranscriptAug 17 2018, 11:50 AM Comment Actions It would help review if you could describe the exact situation this happens. I am wondering:
Comment Actions Please read https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files Generally we should have a short test that just exposes the problem (such short tests also often intuitively demonstrate the problem that is fixed) rather than hundreds of lines of dumped IR. Comment Actions Hi Matthias Thanks for the pointer on MIR tests. I stripped out all the unnecessary IR, but I left the MIR as it was because that was already the result of bugpointing the original test case. Is that comment better for understanding the situation in which it happens?
The main liverange would never be immediately dead at the copy because the dead copy would have been removed before getting to removePartialRedundancy, right?
I don't know the answer to that. But note that it is only valid to remove the endpoint at the same index as the copy because we know it must come from a dead def, rather than a normal use at the same place. We know that because the copy is a full copy. That same thing might not apply in other cases of pruneValue()/extendToIndices(). Comment Actions LGTM with nitpicks addressed.
This revision is now accepted and ready to land.Aug 22 2018, 10:47 AM tpr marked an inline comment as done. Comment ActionsV3: Addressed minor review comments. Closed by commit rL340549: [RegisterCoalescer] Fix for assert in removePartialRedundancy (authored by tpr). · Explain WhyAug 23 2018, 10:29 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 161294 lib/CodeGen/RegisterCoalescer.cpp
test/CodeGen/AMDGPU/regcoalescing-remove-partial-redundancy-assert.mir
|
Using SlotIndex::isSameInstr() would make the intention more clear.