[PATCH] D130290: [MachineVerifier] add checks for INLINEASM_BR

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:882
+
+      const BasicBlock *IndirectTarget = MO.getBlockAddress()->getBasicBlock();
+
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > arsenm wrote:
> > > > > > > > > > > Why is this based on the IR block?
> > > > > > > > > > Because AFAIK, there is no corresponding `BlockAddress` `Constant` in the MIR level, so the `MachineInst` uses a `BlockAddress` `Constant` which is just a tuple of (`Function`, `BasicBlock`). What would be helpful here would be if `BlockAddress` `Constants` were lowered to something new (doesn't exist, yet) that is a tuple of (`MachineFunction`, `MachineBasicBlock`).
> > > > > > > > > > 
> > > > > > > > > > The existing design seems brittle, because IIUC, there's a one-to-many relationship between `BasicBlock` and `MachineBasicBlock`. A `MachineBasicBlock` can refer to a single `BasicBlock`.
> > > > > > > > > > 
> > > > > > > > > > Perhaps that's why in my TODO below that we _don't_ have a mapping of `BasicBlock` to `MachineBasicBlock`; because they are not one-to-one (unless I'm wrong).
> > > > > > > > > `MachineOperand::getMBB()` is almost what I want here, but that corresponds to a different `MachineOperandType` (`MO_MachineBasicBlock`) than what is being used for `INLINEASM_BR` (`MO_BlockAddress`).  Maybe `MO_BlockAddress` needs to die?
> > > > > > > > > Because AFAIK, there is no corresponding BlockAddress Constant in the MIR level
> > > > > > > > 
> > > > > > > > So that's wrong; there _is_ a corresponding `MachineOperandType`: `MO_MachineBasicBlock`.  It's just that `INLINEASM_BR` doesn't use those.
> > > > > > > It probably makes sense to change INLINEASM_BR to use MO_MachineBasicBlock operands instead of MO_BlockAddress operands; that basically corresponds to the IR change we made to remove blockaddress operands.
> > > > > > > 
> > > > > > > (Re: the one-to-many thing, see D124697.)
> > > > > > Something like:
> > > > > > 
> > > > > > ```
> > > > > > diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
> > > > > > index c312fca616a5..3d2ff957eb77 100644
> > > > > > --- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
> > > > > > +++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
> > > > > > @@ -5287,14 +5287,15 @@ TargetLowering::ParseConstraints(const DataLayout &DL,
> > > > > >      case InlineAsm::isInput:
> > > > > >        OpInfo.CallOperandVal = Call.getArgOperand(ArgNo);
> > > > > >        break;
> > > > > > -    case InlineAsm::isLabel:
> > > > > > -      OpInfo.CallOperandVal =
> > > > > > -          cast<CallBrInst>(&Call)->getBlockAddressForIndirectDest(LabelNo);
> > > > > > +    case InlineAsm::isLabel: {
> > > > > > +      const BlockAddress *BA =
> > > > > > +        cast<CallBrInst>(&Call)->getBlockAddressForIndirectDest(LabelNo);
> > > > > > +      OpInfo.CallOperandVal = BA->getBasicBlock();
> > > > > >        OpInfo.ConstraintVT =
> > > > > > -          getAsmOperandValueType(DL, OpInfo.CallOperandVal->getType())
> > > > > > -              .getSimpleVT();
> > > > > > +        getAsmOperandValueType(DL, BA->getType()).getSimpleVT();
> > > > > >        ++LabelNo;
> > > > > >        continue;
> > > > > > +    }
> > > > > >      case InlineAsm::isClobber:
> > > > > >        // Nothing to do.
> > > > > >        break;
> > > > > > ```
> > > > > > seems to work. I need to update a few tests
> > > > > > ```
> > > > > > Failed Tests (13):
> > > > > >   LLVM :: CodeGen/AArch64/callbr-asm-label.ll
> > > > > >   LLVM :: CodeGen/AArch64/callbr-asm-obj-file.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-bb-exports.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-destinations.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-instr-scheduling.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-label-addr.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-obj-file.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-outputs.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm-sink.ll
> > > > > >   LLVM :: CodeGen/X86/callbr-asm.ll
> > > > > >   LLVM :: CodeGen/X86/inline-asm-pic.ll
> > > > > >   LLVM :: CodeGen/X86/shrinkwrap-callbr.ll
> > > > > >   LLVM :: CodeGen/X86/tail-dup-asm-goto.ll
> > > > > > ```
> > > > > > but it just looks like now we emit `jmp .LBB0_2` rather than `jmp .Ltmp0`.  I haven't verified that's correct but it seems innocuous; will do so after lunch, update tests, and post patch.
> > > > > IIRC, I think we added code to retain/emit symbols like `.Ltmp0:` for `# Block address taken`. We _might_ be able to remove that with this change. Maybe not, NBD either way.
> > > > ah, `llvm/test/CodeGen/AArch64/callbr-asm-obj-file.ll` demonstrates that retaining the label needs to be moved from the `.Ltmp` ones to the `.LBBX` ones. WIP
> > > Ok, that's actually a small but nice cleanup to `AsmPrinter::emitBasicBlockStart` (llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp).
> > > It probably makes sense to change INLINEASM_BR to use MO_MachineBasicBlock operands instead of MO_BlockAddress operands; that basically corresponds to the IR change we made to remove blockaddress operands.
> > 
> > One thing I can't figure out...INLINEASM_BR is constructed from `SelectionDAGBuilder::visitInlineAsm` llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp. But AFAICT there's not SDNode 
> > It probably makes sense to change INLINEASM_BR to use MO_MachineBasicBlock operands instead of MO_BlockAddress operands; that basically corresponds to the IR change we made to remove blockaddress operands.
> 
> It seems like there's no corresponding SDValue for MO_MachineBasicBlock, IIUC?  So I _think_ the operand has to remain an IR `BasicBlock`?
SelectionDAG::getBasicBlock() builds an SDValue for an MBB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130290