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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a subscriber: stefanp.
nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1683
   }
+  if (const auto *BB = dyn_cast<BasicBlock>(V))
+    return DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5083
 
-    if (Op.getNode() && Op.getOpcode() == ISD::TargetBlockAddress)
+    if (isa<BasicBlock>(v) || isa<BlockAddress>(v)) {
+      OpInfo.ConstraintCode = "i";
----------------
jyknight wrote:
> I'm still confused as to why this deals in both BlockAddress and BasicBlock types -- why aren't the values all BlockAddresses?
`llvm/test/CodeGen/X86/asm-block-labels.ll` with assert without the check for `BasicBlock`.
```
Value type is non-standard value, Other.
UNREACHABLE executed at /android0/llvm-project/llvm/include/llvm/Support/MachineValueType.h:865!
...
2.	Running pass 'X86 DAG->DAG Instruction Selection' on function '@bar'
...
 #8 0x00000000030d9abc getCopyToParts(llvm::SelectionDAG&, llvm::SDLoc const&, llvm::SDValue, llvm::SDValue*, unsigned int, llvm::MVT, llvm::Value const*, llvm::Optional<unsigned int>, llvm::ISD::NodeType) SelectionDAGBuilder.cpp:0:0
 #9 0x00000000030d9207 llvm::RegsForValue::getCopyToRegs(llvm::SDValue, llvm::SelectionDAG&, llvm::SDLoc const&, llvm::SDValue&, llvm::SDValue*, llvm::Value const*, llvm::ISD::NodeType) const (/android0/llvm-project/llvm/build/bin/llc+0x30d9207)
#10 0x00000000030feb26 llvm::SelectionDAGBuilder::visitInlineAsm(llvm::CallBase const&, llvm::BasicBlock const*) (/android0/llvm-project/llvm/build/bin/llc+0x30feb26)
#11 0x00000000030dca47 llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) (/android0/llvm-project/llvm/build/bin/llc+0x30dca47)
```
ie. `call` (and technically `callbr`) can accept a `label` operand; not just a `blockaddress`. Why are `label` operands distinct from `blockaddress`es? /me shrugs.


================
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
----------------
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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688