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

[Bill Wendling via Phabricator via llvm-commits]

void accepted this revision.
void added inline comments.
This revision is now accepted and ready to land.


================
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:
> > craig.topper wrote:
> > > 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.
> > There is a use of it in the `.td` files:
> > 
> > ```
> > $ grep tblockaddress
> > llvm/include/llvm/Target/TargetSelectionDAG.td:def tblockaddress: SDNode<"ISD::TargetBlockAddress",  SDTPtrLeaf, [],
> >   ...
> > ```
> > 
> > My concern is that changing this will affect code gen in peculiar ways. Is it possible to retain the previous use of the `TargetBlockAddress`? Or could we transmography the node from a `ISD::BlockAddress` to `ISD::TargetBlockAddress`?
> > That said PPCTargetLowering::PPCTargetLowering is doing something "Custom" with ISD::BlockAddress but not ISD::TargetBlockAddress. I wonder if it should be?
> 
> Ok, so IIUC, it looks like the backend specific TargetLowering methods "raise" or "promote" `ISD::BlockAddress`es to `ISD::TargetBlockAddress`es.  I //suspect// it's more correct for this part of ISEL (`SelectionDAGBuilder::visitInlineAsm`) to lower to `ISD::BlockAddress` and then let the target specific backends raise those to `ISD::TargetBlockAddress`es correctly.  But I will admit I'm pretty unfamiliar with all this, so I probably don't know what I'm talking about.
> 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.

I think you're on the `git blame` output for that one. ;-)

Okay, I'll back off on this. It sounds like it was more an attempt to say "No, we're sure we have the correct thing here."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115688