[PATCH] D88823: [CodeGen][TailDuplicator] Don't duplicate blocks with INLINEASM_BR

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

>> %9:gr32 = PHI %0:gr32, %bb.1, %5:gr32, %bb.2
>
> The problem is this PHI here. We don't have a way to resolve PHI nodes in the entry block to an indirect destination. (This is the reason we don't allow "asm goto" outputs on the indirect paths.) When we go to resolve this PHI node, there's no place to put the put the values that works in all situations; we can't split the critical edges and it's not valid to place the resolved instructions at the ends of the predecessor blocks.

We're also not using "asm goto" outputs in this test case, so I still don't understand why this PHI is problematic.

> We don't have a way to resolve PHI nodes in the entry block to an indirect destination.

We should be able to catch cases where we have a `MachineBasicBlock` whose address is taken, yet starts with a `phi`, right?  Then an assert or `-verify-machineinstrs` check should help avoid producing such cases.

  diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
  index 870b4606e51d..dbd1e71c8619 100644
  --- a/llvm/lib/CodeGen/MachineVerifier.cpp
  +++ b/llvm/lib/CodeGen/MachineVerifier.cpp
  @@ -2408,6 +2408,11 @@ void MachineVerifier::checkPHIOps(const MachineBasicBlock &MBB) {
               !PrInfo.isLiveOut(MO0.getReg()))
             report("PHI operand is not live-out from predecessor", &MO0, I);
         }
  +
  +      if (MBB.isInlineAsmBrIndirectTarget())
  +        for (const MachineInstr &MI: Pre)
  +          if (MI.getOpcode() == TargetOpcode::INLINEASM_BR)
  +            report("PHI in successor of INLINEASM_BR predecassor", &MO1, I);
       }
   
       // Did we see all predecessors?

fails in the existing test cases:

  Failed Tests (2):
    LLVM :: CodeGen/X86/callbr-asm-outputs.ll
    LLVM :: CodeGen/X86/callbr-asm-phi-placement.ll

Maybe there's a more specific case that's problematic?

---

I feel like we SHOULD be able to allow tail dup in this basic case:

  declare void @foo()
  declare void @bar()
  
  define i32 @baz(i1 %arg_0) {
    br i1 %arg_0, label %bb1, label %bb2
  bb1:
    call void @foo()
    br label %bb3
  bb2:
    call void @bar()
    br label %bb3
  bb3:
    %x = phi i32 [0, %bb1], [1, %bb2]
    callbr void asm sideeffect "jmp ${2:l}", "i,i,X,~{dirflag},~{fpsr},~{flags}"(i32 42, i1 false, i8* blockaddress(@baz, %bb4))
            to label %bb5 [label %bb4]
  bb4:
    br label %bb5
  bb5:
    ret i32 %x
  }

Prior to this patch and James' patch, we would tail duplicate that.  With this change, we wont. (It looks to me like we get the codegen of that case correct).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88823/new/

https://reviews.llvm.org/D88823