[PATCH] D130316: [SelectionDAG] make INLINEASM_BR use MachineBasicBlocks instead of BlockAddresses

Fri Jul 22 14:30:20 PDT 2022

nikic added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5294-5295
+      OpInfo.CallOperandVal = BA->getBasicBlock();
       OpInfo.ConstraintVT =
-          getAsmOperandValueType(DL, OpInfo.CallOperandVal->getType())
-              .getSimpleVT();
+        getAsmOperandValueType(DL, BA->getType()).getSimpleVT();
       ++LabelNo;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > nikic wrote:
> > > > > nickdesaulniers wrote:
> > > > > > Not sure this is quite right, but also not sure what it should be.  Maybe the reviewers do?
> > > > > `BA` has an `i8*` type, so can directly use that?
> > > > `Type::getInt8PtrTy` seems to cause crashes...cant tell what type is actually being used; everything looks like opaque ptrs...
> > > branch-folder is messing something up...
> > ahoy! Both TailDuplicator and BranchFolding were relying on whether an MBB has its address taken. After this change, they wont, so those cases additionally need to check for isInlineAsmBrIndirectTarget. With that added, we're working AND the .Ltmp labels are gone (good)!  Now just to clean up the test churn after lunch, but I think this will all work and look nice. Stay tuned.
> The last wart on an otherwise great simplification is getting the context for `Type::getInt8PtrTy`. Thoughts on how I can improve this?
I think you can just use `getProgramPointerTy(DL)` to directly get the right MVT. No need to indirect via IR types.

You should also be able to drop the getBlockAddressForIndirectDest() method.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130316