[PHIElimination] Account for INLINEASM_BR when inserting kills

[foad]

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.

[foad]

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

[MatzeB]

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.

[MatzeB]

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

[foad]

In D110834#3040559, @MatzeB wrote:

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]

Switch to a MIR test case.

[foad]

In D110834#3042425, @foad wrote:

In D110834#3040559, @MatzeB wrote:

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.

llvm/lib/CodeGen/PHIElimination.cpp

if (LV && !SrcUndef &&
       // value!
 
       // In our final twist, we have to decide which instruction kills the
       // register.  In most cases this is the copy, however, terminator
       // instructions at the end of the block may also use the value. In this
       // case, we should mark the last such terminator as being the killing
       // block, not the copy.
       MachineBasicBlock::iterator KillInst = opBlock.end();
-      MachineBasicBlock::iterator FirstTerm = opBlock.getFirstTerminator();
-      for (MachineBasicBlock::iterator Term = FirstTerm;
-          Term != opBlock.end(); ++Term) {
+      for (MachineBasicBlock::iterator Term = InsertPos; Term != opBlock.end();
+           ++Term) {
         if (Term->readsRegister(SrcReg))
           KillInst = Term;
       }
 
       if (KillInst == opBlock.end()) {
         // No terminator uses the register.
 
         if (reusedIncoming || !IncomingReg) {
           // We may have to rewind a bit if we didn't insert a copy this time.
-          KillInst = FirstTerm;
+          KillInst = InsertPos;
           while (KillInst != opBlock.begin()) {
             --KillInst;
             if (KillInst->isDebugInstr())
               continue;
             if (KillInst->readsRegister(SrcReg))
               break;
           }
         } else {
if (LIS) {
           if (VNI && VNI->def != startIdx) {
             isLiveOut = true;
             break;
           }
         }
 
         if (!isLiveOut) {
           MachineBasicBlock::iterator KillInst = opBlock.end();
-          MachineBasicBlock::iterator FirstTerm = opBlock.getFirstTerminator();
-          for (MachineBasicBlock::iterator Term = FirstTerm;
+          for (MachineBasicBlock::iterator Term = InsertPos;
                Term != opBlock.end(); ++Term) {
             if (Term->readsRegister(SrcReg))
               KillInst = Term;
           }
 
           if (KillInst == opBlock.end()) {
             // No terminator uses the register.
 
             if (reusedIncoming || !IncomingReg) {
               // We may have to rewind a bit if we didn't just insert a copy.
-              KillInst = FirstTerm;
+              KillInst = InsertPos;
               while (KillInst != opBlock.begin()) {
                 --KillInst;
                 if (KillInst->isDebugInstr())
                   continue;
                 if (KillInst->readsRegister(SrcReg))
                   break;
               }
             } else {

llvm/test/CodeGen/X86/callbr-asm-kill.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -O2 -stop-after=phi-node-elimination < %s | FileCheck %s
 
 declare void @foo(i8*)
 
-; FIXME: The COPY from [[MOV64rm]] should not be killed, because there is a
-; subsequent use of [[MOV64rm]] in the INLINEASM_BR instruction.
+; Check that the COPY from [[MOV64rm]] is not killed, because there is a
+; subsequent use of [[MOV64rm]] in the INLINEASM_BR instruction which should be
+; killed instead.
 define void @test1(i8* %arg, i8** %mem) nounwind {
   ; CHECK-LABEL: name: test1
   ; CHECK: bb.0.entry:
   ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT:   liveins: $rdi, $rsi
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[COPY:%[0-9]+]]:gr64 = COPY killed $rsi
   ; CHECK-NEXT:   [[COPY1:%[0-9]+]]:gr64 = COPY killed $rdi
   ; CHECK-NEXT:   [[COPY2:%[0-9]+]]:gr64 = COPY killed [[COPY1]]
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1.loop (address-taken):
   ; CHECK-NEXT:   successors: %bb.2(0x80000000), %bb.1(0x00000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:gr64 = COPY killed [[COPY2]]
   ; CHECK-NEXT:   [[MOV64rm:%[0-9]+]]:gr64 = MOV64rm [[COPY]], 1, $noreg, 0, $noreg :: (load (s64) from %ir.mem)
   ; CHECK-NEXT:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
   ; CHECK-NEXT:   $rdi = COPY killed [[COPY3]]
   ; CHECK-NEXT:   CALL64pcrel32 target-flags(x86-plt) @foo, csr_64, implicit $rsp, implicit $ssp, implicit killed $rdi, implicit-def $rsp, implicit-def $ssp
   ; CHECK-NEXT:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
-  ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:gr64 = COPY killed [[MOV64rm]]
-  ; CHECK-NEXT:   INLINEASM_BR &"", 9 /* sideeffect mayload attdialect */, 196654 /* mem:m */, [[MOV64rm]], 1, $noreg, 0, $noreg, 13 /* imm */, blockaddress(@test1, %ir-block.loop)
+  ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:gr64 = COPY [[MOV64rm]]
+  ; CHECK-NEXT:   INLINEASM_BR &"", 9 /* sideeffect mayload attdialect */, 196654 /* mem:m */, killed [[MOV64rm]], 1, $noreg, 0, $noreg, 13 /* imm */, blockaddress(@test1, %ir-block.loop)
   ; CHECK-NEXT:   JMP_1 %bb.2
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2.end:
   ; CHECK-NEXT:   RET 0
 entry:
   br label %loop
 
 loop: