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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8569
       OpInfo.CallOperandVal = Call.getArgOperand(ArgNo++);
-
-      // Process the call argument. BasicBlocks are labels, currently appearing
-      // only in asm's.
-      if (isa<CallBrInst>(Call) &&
-          ArgNo - 1 >= (cast<CallBrInst>(&Call)->arg_size() -
-                        cast<CallBrInst>(&Call)->getNumIndirectDests() -
-                        NumMatchingOps) &&
-          (NumMatchingOps == 0 ||
-           ArgNo - 1 <
-               (cast<CallBrInst>(&Call)->arg_size() - NumMatchingOps))) {
-        const auto *BA = cast<BlockAddress>(OpInfo.CallOperandVal);
-        EVT VT = TLI.getValueType(DAG.getDataLayout(), BA->getType(), true);
-        OpInfo.CallOperand = DAG.getTargetBlockAddress(BA, VT);
-      } else if (const auto *BB = dyn_cast<BasicBlock>(OpInfo.CallOperandVal)) {
-        OpInfo.CallOperand = DAG.getBasicBlock(FuncInfo.MBBMap[BB]);
-      } else {
-        OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
-      }
-
+      OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
       EVT VT = OpInfo.getCallOperandValEVT(*DAG.getContext(), TLI,
----------------
void wrote:
> The old code uses `DAG.getTargetBlockAddress(...)`. This new code uses `DAG.getBlockAddress(...)`. What's the difference?
`BlockAddressSDNode::classof` is interesting; there's one node with two possible opcodes; `ISD::BlockAddress` and `ISD::TargetBlockAddress`.

`SelectionDAG:: getTargetBlockAddress` is also interesting, as it is implemented in terms of a call to `SelectionDAG::getBlockAddress` with the optional `isTarget` parameter set to `true` (defaulting to `false` otherwise).

So this change as is (Diff 395500) is not setting the `isTarget` flag, which just changes the opcode of the `SDNode`.  The only places in the codebase where I observe `ISD::TargetBlockAddress` being handled differently from `ISD::BlockAddress` are:
1. `TargetLowering::ComputeConstraintToUse` which I actually unify in this diff, see below.
2. `SelectionDAGISel::SelectCodeCommon`. I'm not quite sure yet what this code is doing, will read through it a couple times now. Perhaps that's what's causing the difference in `llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll` and `llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll`? That said `PPCTargetLowering::PPCTargetLowering` is doing something "Custom" with `ISD::BlockAddress` but not `ISD::TargetBlockAddress`. I wonder if it should be? Rephrased: should `PPCTargetLowering::LowerOperation` have an additional `case ISD::TargetBlockAddress` that also dispatches to `PPCTargetLowering::LowerBlockAddress` AND `PPCTargetLowering::PPCTargetLowering` mark `ISD::TargetBlockAddress` as having custom handling? Lots (all?) backends seem to follow this pattern though? [[ https://gist.github.com/nickdesaulniers/e13f16a0d98ce59afbe795298827e6fe | Messing with that creates a pretty spectacular failure though. ]] 
3. `SDNode::getOperationName` which converts an `SDNode` to a `std::string` based on the opcode. Not interesting/relevant.

---

Just another thought. If we're ok matching GCC with D115471, I could actually rebase this on that, then restore the `ArgNo` based checking here back to what it was when `asm goto` support was first introduced. Then we'd still be using `TargetBlockAddress`es rather than `BlockAddress`es.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688