[PATCH] D115688: [SelectionDAG] treat X constrained labels as i for asm

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

LG, with a few minor comments.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1683
   }
+  if (const auto *BB = dyn_cast<BasicBlock>(V))
+    return DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
----------------
nickdesaulniers wrote:
> jyknight wrote:
> > How does BasicBlock get here, vs BlockAddress?
> Note that this method also handles `BlockAddress`es above in the conditional for `isa<Constant>` (`BlockAddress` `isa<Constant>`; `BasicBlock` `!isa<Constant>` on line 1622.
> 
> Without this check, `llvm/test/CodeGen/X86/asm-block-labels.ll` asserts:
> ```
> Can't get register for value!
> UNREACHABLE executed at /android0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1685!
> ...
> 2.	Running pass 'X86 DAG->DAG Instruction Selection' on function '@bar'
> ...
>  #7 0x00000000030f2e89 llvm::SelectionDAGBuilder::getValueImpl(llvm::Value const*) (/android0/llvm-project/llvm/build/bin/llc+0x30f2e89)
>  #8 0x00000000030f1bbf llvm::SelectionDAGBuilder::getValue(llvm::Value const*) (/android0/llvm-project/llvm/build/bin/llc+0x30f1bbf)
>  #9 0x00000000030fcd7b llvm::SelectionDAGBuilder::visitInlineAsm(llvm::CallBase const&, llvm::BasicBlock const*) (/android0/llvm-project/llvm/build/bin/llc+0x30fcd7b)
> ```
> Note that `@bar` is only composed of `call` (no `callbr`); one statement has a `label` operand.
Taking the label directly as a value seems like something we should not be supporting at all. Yet, since I guess we already did, OK, fine.


================
Comment at: llvm/test/CodeGen/AArch64/inlineasm-X-constraint.ll:130
 ; CHECK-LABEL: f7
 ; CHECK: bl
 define void @f7() {
----------------
jyknight wrote:
> Can now "CHECK: bl bb"?
Still outstanding: would be nice to include the target in the check line, verifying that it's an immediate not a register.


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll:84
 ; PPC64LE-NEXT:    #NO_APP
-; PPC64LE-NEXT:  # %bb.1: # %return
+; PPC64LE-NEXT: .LBB3_1: # %return
 ; PPC64LE-NEXT:    extsw r3, r3
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > jyknight wrote:
> > > The asm fallthrough grew a label it didn't need before? And...that affected the need to save/restore lr or something?
> > > 
> > > Hmm...wait...it sure looks like the expected code in this test was demonstrating a bug...! It wasn't saving/restoring lr (mflr/mtlr) around the callbr, but the callbr said it clobbers lr! That doesn't seem right -- despite the test asserting it...
> > > 
> > > OK...but why did this change fix that?
> > + @stefanp @nemanjai 
> So I thought this may be because of the changes to `SelectionDAGBuilder::visitInlineAsm` here (in D115688 Diff 395500).
> 
> As a test, I rebased locally D115471 to appear first, rather than last in the patch set, rebasing this on that. That allowed me to remove the `NumMatching` logic, but keep basically the same version as when `callbr` support was initially added; if the indirect dests are always the final inputs, the ambiguity alluded to in this patch no longer exists.
> 
> I know you weren't enthused by D115471, but I think landing that first actually simplifies this patch (the diff to llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll here goes away, and we return `SelectionDAGBuilder::visitInlineAsm` back to how it was prior to output support).  For that reason, I'd like you to reconsider D115471.
> 
> > Hmm...wait...it sure looks like the expected code in this test was demonstrating a bug...! It wasn't saving/restoring lr (mflr/mtlr) around the callbr, but the callbr said it clobbers lr! That doesn't seem right -- despite the test asserting it...
> 
> I left an additional comment on https://reviews.llvm.org/D101657.
> 
> > OK...but why did this change fix that?
> 
> I'm still not precisely sure why this change tickled that, but perhaps let's fix that first? https://reviews.llvm.org/D116424
I assume the diff in this file has gone away now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688