[LiveVariables] Mark PhysReg implicit-def MachineOperands of INLINEASM_BR as LiveOut

[nickdesaulniers]

Summary

D78586 implements good sanity checks in MachineVerifier that for each
LiveIn register to a MachineBasicBlock, that at least one predecessor
MachineBasicBlock defines the register as LiveOut.

The PhysReg MachineOperands for INLINEASM_BR MachineInstrs were being
implicit-def'd but marked dead (i.e. no uses in local MachineBasicBlock)
by live-vars.

ScheduleDAGSDNodes::EmitSchedule() splits the MachineBasicBlock
containing the INLINEASM_BR, post that MachineInstr, and marks the
LiveIns. But there's no marking of LiveOuts. If a use is visible within
the same MachineBasicBlock, live-vars will calculate the LiveOuts
correctly.

Since the uses aren't visible in the same MachineBasicBlock as the
INLINEASM_BR MachineInstr, teach LiveVariables::runOnBlock() to special
case INLINEASM_BR's PhysReg defs, so that they're not marked dead.

Implementing TCOPY (D75098) should also make the uses visible to
live-vars as well, at which point, this change to LiveVariables should
be reverted (but not the tests).

[efriedma]

llvm/lib/CodeGen/LiveVariables.cpp

24

I'm concerned about the "allowing it to do a single local analysis to resolve physical register lifetimes in each basic block" part. Is this change enough to actually make the liveness reliable? In particular, I'm concerned what might happens if the fallthrough successor of an INLINEASM_BR has other predecessors. (Or is that impossible?)

619

How is EarlyClobber related to this?

[void]

llvm/lib/CodeGen/LiveVariables.cpp

24

Drive-by comment:

INLINEASM_BR's fallthrough successor can have multiple predecessors, at least in theory (I don't believe there's a validation check preventing it). E.g.:

a(bool b) {
  if (b)
    asm goto("" : : : : indirect1);
  else
    asm goto("" : : : : indirect2);
  // Both "asm goto"s fallthrough to here.
indirect1:
...
indirect2:
...
}

In the IR, I believe clang generates a fallthrough block for each "asm goto" even though they both end up falling through to the same place. But because of general transformations, those can be elided of course, especially when converted to machine instructions.

My original implementation created a separate "copy" MBB that has only one predecessor---the block with the INLINEASM_BR instruction. It acts as a block that would be created during critical edge splitting.

[nickdesaulniers]

Superseded by D139565.

llvm/lib/CodeGen/LiveVariables.cpp

 // This class computes live variables using a sparse implementation based on
 // the machine code SSA form.  This class computes live variable information for
 // each virtual and _register allocatable_ physical register in a function.  It
 // uses the dominance properties of SSA form to efficiently compute live
 // variables for virtual registers, and assumes that physical registers are only
 // live within a single basic block (allowing it to do a single local analysis
 // to resolve physical register lifetimes in each basic block).  If a physical
 // register is not register allocatable, it is not tracked.  This is useful for
 // things like the stack pointer and condition codes.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/LiveVariables.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
if (SuccMBB->isEHPad())
       continue;
     for (const auto &LI : SuccMBB->liveins()) {
       if (!TRI->isInAllocatableClass(LI.PhysReg))
         // Ignore other live-ins, e.g. those that are live into landing pads.
         LiveOuts.insert(LI.PhysReg);
     }
   }
 
+  // INLINEASM_BR's define physical registers are live, though we don't see
+  // their uses in the split copy block.
+  for (const MachineInstr &MI : MBB->terminators())
+    if (MI.getOpcode() == TargetOpcode::INLINEASM_BR)
+      for (const MachineOperand &MO : MI.operands())
+        if (MO.isReg() && Register::isPhysicalRegister(MO.getReg()) &&
+            MO.isDef() && !MO.isDead() && !MO.isEarlyClobber())
+          LiveOuts.insert(MO.getReg());
+
   // Loop over PhysRegDef / PhysRegUse, killing any registers that are
   // available at the end of the basic block.
   for (unsigned i = 0; i != NumRegs; ++i)
     if ((PhysRegDef[i] || PhysRegUse[i]) && !LiveOuts.count(i))
       HandlePhysRegDef(i, nullptr, Defs);
 }
 
 bool LiveVariables::runOnMachineFunction(MachineFunction &mf) {

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

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=i686-- -print-after=livevars -stop-after=livevars < %s 2>&1 | FileCheck --check-prefix=LIVE-CHECK %s
 
 ; A test for asm-goto output
 
 define i32 @test1(i32 %x) {
 ; CHECK-LABEL: test1:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; CHECK-NEXT:    addl $4, %eax

 entry:
   %cmp = icmp slt i32 %out1, %out2
   br i1 %cmp, label %if.then, label %if.else
 
 if.then:                                          ; preds = %entry
   %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "={si},={di},r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
           to label %if.end [label %label_true, label %return]
 
+; Test that these physical registers are not `implicit-def dead`, i.e. both
+; defined by this instruction, and yet dead/no users.
+
+; LIVE-CHECK: INLINEASM_BR{{.*}}implicit-def $esi,{{.*}}implicit-def $edi,
+
 if.else:                                          ; preds = %entry
   %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "={si},={di},r,r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i32 %out2, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
           to label %if.end [label %label_true, label %return]
 
+; LIVE-CHECK: INLINEASM_BR{{.*}}implicit-def $esi,{{.*}}implicit-def $edi,
+
 if.end:                                           ; preds = %if.else, %if.then
   %.sink11 = phi { i32, i32 } [ %0, %if.then ], [ %1, %if.else ]
   %asmresult3 = extractvalue { i32, i32 } %.sink11, 0
   %asmresult4 = extractvalue { i32, i32 } %.sink11, 1
   %add = add nsw i32 %asmresult4, %asmresult3
   br label %return
 
 label_true:                                       ; preds = %if.else, %if.then