Page MenuHomePhabricator

[SelectionDAG] make INLINEASM_BR use MachineBasicBlocks instead of BlockAddresses
ClosedPublic

Authored by nickdesaulniers on Jul 21 2022, 4:50 PM.

Details

Summary

As part of re-architecting callbr to no longer use blockaddresses
(https://reviews.llvm.org/D129288), we don't really need them in MIR.
They make comparing MachineBasicBlocks of indirect targets during
MachineVerifier a PITA.

Suggested by @efriedma from the discussion:
https://reviews.llvm.org/D130290#3669531

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 4:50 PM
nickdesaulniers requested review of this revision.Jul 21 2022, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 4:50 PM
nickdesaulniers planned changes to this revision.Jul 21 2022, 5:03 PM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3639–3643

I'm going to test these out, too. But RFC; comments welcome.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

Not sure this is quite right, but also not sure what it should be. Maybe the reviewers do?

llvm/test/CodeGen/X86/tail-dup-asm-goto.ll
11

I think I should pre-commit an initial run of update_mir_test_checks.py on this...

  • SelectionDAGBuilder::visitCallBr() no longer needs to call MachineBasicBlock::setHasAddressTaken()

We might also be able to remove some of the handling in
EmitGCCInlineAsmStr <llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp>

  • commit 83d690a14980 ("Don't rely on 'l'(ell) modifiers to indicate a label reference")
  • commit 1b104388752f ("[MC] Don't recreate a label if it's already used")
arsenm added inline comments.Jul 21 2022, 6:30 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3638–3641

Comments can be removed after D130323?

barannikov88 added inline comments.
llvm/test/CodeGen/AArch64/callbr-asm-label.ll
10

I'm a bit out of context, so the question might be silly, sorry.
Shouldn't this temporary label no longer be emitted? It used to be referenced, but no longer is.

nikic added inline comments.Jul 22 2022, 12:07 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5293

This is a roundabout way of saying cast<CallBrInst>(&Call)->getIndirectDest(LabelNo) -- unless the fact that a BlockAddress on the IR level is created is still important for something?

5294–5295

BA has an i8* type, so can directly use that?

llvm/test/CodeGen/AArch64/callbr-asm-label.ll
10

It's not a silly question; it's a great question.

I've been trying to track that down, as I don't know the answer myself.

IIRC, we did previously have an issue with labels disappearing out from under us. @void solved that in either

  • commit 83d690a14980 ("Don't rely on 'l'(ell) modifiers to indicate a label reference")
  • commit 1b104388752f ("[MC] Don't recreate a label if it's already used")

I've already found that the latter is no longer necessary, hence https://reviews.llvm.org/D130323, but that's not what was retaining these additional symbols.

(or maybe it was a third commit, maybe 79176a2542d03107b90613c84f18ccba41ad8fa8?)

So I would like to find the answer to this question, and perhaps remove the duplicate labels. But it would be a minor cosmetic fix; I'm not going to spend too much time on it since this patch is just a chain in a longer series that we're ultimately trying to fix. Fixing duplicate labels is nice to have, but slightly orthogonal.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

Type::getInt8PtrTy seems to cause crashes...cant tell what type is actually being used; everything looks like opaque ptrs...

nickdesaulniers marked an inline comment as done.
  • use CallBrInst::getIndirectDest rather than BlockAddress::getBasicBlock for OpInfo.CallOperandVal
nickdesaulniers marked an inline comment as done and an inline comment as not done.
  • prefer MachineBasicBlock::setLabelMustBeEmitted in SelectionDAGBuilder::visitCallBr over an additional check for MachineBasicBlock::isInlineAsmBrIndirectTarget in AsmPrinter::shouldEmitLabelForBasicBlock
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5293

Actually, it might be for some of the follow up reverts. I wonder if this is what's creating all of the .Ltmp labels that no longer seem necessary...

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

branch-folder is messing something up...

nickdesaulniers planned changes to this revision.Jul 22 2022, 1:20 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

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.

  • get rid of the stupid .Ltmp double labels
nickdesaulniers marked 2 inline comments as done.Jul 22 2022, 2:23 PM

Ok, this is ready for review/testing. It's part of a series:

  1. https://reviews.llvm.org/D130316
  2. https://reviews.llvm.org/D130323
  3. https://reviews.llvm.org/D130391

Then 4 (https://reviews.llvm.org/D130290) which I will rebase once this lands.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

The last wart on an otherwise great simplification is getting the context for Type::getInt8PtrTy. Thoughts on how I can improve this?

nikic added inline comments.Jul 22 2022, 2:30 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

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.

barannikov88 added inline comments.Jul 22 2022, 2:32 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5297

Debugging leftover, I guess?

barannikov88 added inline comments.Jul 22 2022, 2:40 PM
llvm/test/CodeGen/X86/inline-asm-pic.ll
34

The one here remains.

nickdesaulniers marked 3 inline comments as done.
  • use getProgramPointerTy, del CallBrInst::getBlockAddressForIndirectDest
nickdesaulniers marked an inline comment as done.Jul 22 2022, 2:57 PM
nickdesaulniers added inline comments.
llvm/test/CodeGen/X86/inline-asm-pic.ll
34

This is expected. You'll notice that it's address is taken by the blockaddress constant in @y (Line 57).

My change stops using blockaddress for callbr. It doesn't remove blockaddress from the language; test @y is testing the "address of a label" as an input to the inline asm, which is distinct from the label list used in asm goto.

That said, thanks for triple checking!

nickdesaulniers marked an inline comment as done.Jul 22 2022, 2:57 PM
barannikov88 added inline comments.Jul 22 2022, 3:38 PM
llvm/test/CodeGen/X86/inline-asm-pic.ll
34

Nope, line 57 is a different test.
Anyway, this was just an observation and not a suggestion to look into this specific case. Most parasitic labels are gone, so this is probably something X86-specific, which I believe is out of scope of this patch series.

nikic added a comment.Jul 23 2022, 3:30 AM

I like the change, but I'm not familiar enough with the backend to fully understand the implications. The main thing I'd worry about is that more places that currently check hasAddressTaken() would also need to check isInlineAsmBrIndirectTarget(). E.g. I see more calls to the method in IfConversion. (Would it make sense to stop using blockaddresses, but still mark the MBBs are address-taken?)

nickdesaulniers planned changes to this revision.Jul 25 2022, 10:35 AM

I like the change, but I'm not familiar enough with the backend to fully understand the implications. The main thing I'd worry about is that more places that currently check hasAddressTaken() would also need to check isInlineAsmBrIndirectTarget(). E.g. I see more calls to the method in IfConversion. (Would it make sense to stop using blockaddresses, but still mark the MBBs are address-taken?)

It does seem risky and orthogonal to the goal of this patch. Let me break out the s/setHasAddressTaken/setLabelMustBeEmitted/ change into a distinct child patch. BRB while I rechurn the test changes, again.

  • keep setHasAddressTaken, just add setLabelMustBeEmitted

I like the change, but I'm not familiar enough with the backend to fully understand the implications. The main thing I'd worry about is that more places that currently check hasAddressTaken() would also need to check isInlineAsmBrIndirectTarget(). E.g. I see more calls to the method in IfConversion. (Would it make sense to stop using blockaddresses, but still mark the MBBs are address-taken?)

Done; PTAL. Sorry for the delay, was offline most of last week.

arsenm added inline comments.Aug 5 2022, 9:22 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

getProgramPointerTy is correct but I would expect taking the type directly from the IR would be better (in principle we could have individual functions with a different address space from the DL one, although I think that's currently unsupported)

nickdesaulniers marked an inline comment as done.
  • rebase, remove setting OpInfo.ConstraintVT
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5294–5295

FWICT, this assignment is no longer necessary. Removing it, as it doesn't affect tests.

bumping for review

void accepted this revision.Aug 10 2022, 3:40 PM

This patch seems pretty straight-forward, so I'll add my LGTM here. Please feel free to override that if other reviewers are less sure. :-)

llvm/test/CodeGen/AArch64/callbr-asm-label.ll
10

My memory is hazy on the details, but there were a number of issues with inline asm and labels when dealing with block addresses and basic blocks. The changes you mentioned are large hammers, but *seemed* necessary. There may be better ways to handle the label issue though.

This revision is now accepted and ready to land.Aug 10 2022, 3:40 PM
efriedma accepted this revision.Aug 16 2022, 4:22 PM

LGTM

I just merged D124697; I expect rebasing this on top of that should be trivial, but feel free to ask for another round of review if it isn't for some reason.

  • rebase, update mir tests to remove ir-block-address-taken MBB annotation
This revision was landed with ongoing or failed builds.Aug 17 2022, 9:34 AM
This revision was automatically updated to reflect the committed changes.