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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as not done.
nickdesaulniers added inline comments.


================
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";
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > 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.
> So it looks like a `label` is a `Type` that a `Value` can have, while `blockaddress` is a `Constant` `Value`, IIUC. Still, this holds:
> 
> > call (and technically callbr) can accept a label operand; not just a blockaddress.
> 
> See also the above relationship between `ISD::TargetBlockAddress` and `ISD::BlockAddress`; they're the same `SDNode` (`BlockAddressSDNode`). This hunk of this change is unifying the operands rather than relying one of the possible opcodes for this type of `SDNode`.
> call (and technically callbr) can accept a label operand; not just a blockaddress.

Scratch `and technically callbr`; `Verifier::visitCallBrInst` validates that we do not permit naked label `Value`s in the arg list for `callbr` IIUC.

To re-summarize the answer to the question

> why aren't the values all BlockAddresses?

Because `call` instructions can have arguments that are labels; not just `blockaddress`es, as observed in llvm/test/CodeGen/X86/asm-block-labels.ll.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688