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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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,
----------------
nickdesaulniers wrote:
> 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.
For more information. The "Target" version of nodes are ignored by the isel step of SelectionDAG. They are considered already selected and ignored. This is what the code in SelectNodeCommon is doing.

BlockAddress needs to be replaced before isel finishes, TargetBlockAddress is the only version that can go through InstrEmitter to make MachineIR.

I think usually TargetBlockAddress should be created from BlockAddress as part of lowering. Maybe we were doing something different for asm goto because we weren't really using blockaddress the way it was originally intended? I don't recall if I wrote that part of the code or if I inherited it when I took over the asm goto patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688