This is an archive of the discontinued LLVM Phabricator instance.

[PHIElimination] Account for INLINEASM_BR when inserting kills
ClosedPublic

Authored by foad on Sep 30 2021, 7:45 AM.

Details

Summary

When PHIElimination adds kills after lowering PHIs to COPYs it knows
that some instructions after the inserted COPY might use the same
SrcReg, but it was only looking at the terminator instructions at the
end of the block, not at other instructions like INLINEASM_BR that can
appear after the COPY insertion point.

Since we have already called findPHICopyInsertPoint, which knows about
INLINEASM_BR, we might as well reuse the insertion point that it
calculated when looking for instructions that might use SrcReg.

This fixes a machine verification failure if you force machine
verification to run after PHIElimination (currently it is disabled for
other reasons) when running
test/CodeGen/X86/callbr-asm-phi-placement.ll.

Event Timeline

foad created this revision.Sep 30 2021, 7:45 AM
foad requested review of this revision.Sep 30 2021, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 7:45 AM
foad added a reviewer: MatzeB.Oct 1 2021, 10:22 AM
foad updated this revision to Diff 376588.Oct 1 2021, 11:44 AM

Rebase on a test case that I will pre-commit.

MatzeB accepted this revision.Oct 4 2021, 11:31 AM

Urgh, just learned that INLINEASM_BR was yet another step to turn LLVM machine representation towards an Extended Basic Block model; without that actually being designed/discussed/planned that way. Anyway this LGTM given the current situation.

This revision is now accepted and ready to land.Oct 4 2021, 11:31 AM

Please consider changing the test to use a .mir file.

foad added a comment.Oct 5 2021, 5:53 AM

Please consider changing the test to use a .mir file.

I'm having trouble writing a .mir file containing an INLINEASM_BR instruction because the verifier complains: Bad machine code: MBB has unexpected successors which are not branch targets, fallthrough, EHPads, or inlineasm_br targets.

It seems like whatever flag gets set on an MBB to say it is an inlineasm_br target does not get serialised in textual MIR. @jyknight does that sound right? Any suggestions for writing MIR test cases?

foad updated this revision to Diff 377786.Oct 7 2021, 2:56 AM

Switch to a MIR test case.

foad added a comment.Oct 7 2021, 11:52 AM

Please consider changing the test to use a .mir file.

I'm having trouble writing a .mir file containing an INLINEASM_BR instruction because the verifier complains: Bad machine code: MBB has unexpected successors which are not branch targets, fallthrough, EHPads, or inlineasm_br targets.

It seems like whatever flag gets set on an MBB to say it is an inlineasm_br target does not get serialised in textual MIR. @jyknight does that sound right? Any suggestions for writing MIR test cases?

I fixed that in D111291.

This revision was landed with ongoing or failed builds.Oct 7 2021, 12:04 PM
This revision was automatically updated to reflect the committed changes.