This is an archive of the discontinued LLVM Phabricator instance.

[llvm][CallBrPrepare] add llvm.callbr.landingpad intrinsic
ClosedPublic

Authored by nickdesaulniers on Dec 12 2022, 2:16 PM.

Details

Summary

Insert a new intrinsic call after splitting critical edges, and verify
it. Later commits will update the SSA values to use this new value along
indirect branches rather than the callbr's value, and have SelectionDAG
consume this new value.

Part 2b of
https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/8.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 2:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nickdesaulniers requested review of this revision.Dec 12 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 2:16 PM
llvm/lib/CodeGen/CallBrPrepare.cpp
117

This needs to insert intrinsics even when there is no critical edge to split?

nickdesaulniers planned changes to this revision.Dec 12 2022, 2:36 PM
nickdesaulniers edited the summary of this revision. (Show Details)
  • insert intrinsic call regardless of critical edge splitting
  • move insertion of intrinsic calls out of SplitCriticalEdges (prefer small readable functions)
  • s/Synth/Dest
llvm/include/llvm/IR/Intrinsics.td
623

Looks like I'll need to re-add IntrNoMerge; I'm trying to write tests for llc invocations and I see simplifycfg folding these again...

  • add IntrNoMerge

langref?

llvm/lib/CodeGen/CallBrPrepare.cpp
126

can pass this directly to the constructor

llvm/lib/IR/Verifier.cpp
5774

We should also check that the landingpad is the first instruction (isFirstNonPhi or non debug or some such). Otherwise you could have:

declare i64 @foo(i64)
define i64 @test_callbr_landingpad_not_first_inst() {
entry:
  ; Specifically, output into %rdi
  %0 = callbr i64 asm "", "={di},!i"()
          to label %asm.fallthrough [label %landingpad]

asm.fallthrough:
  ret i64 42

landingpad:
  ; By the calling convention on x86_64-linux-gnu, 42 must be passed in %rdi.
  %foo = call i64 @foo(i64 42)
  %out = call i64 @llvm.callbr.landingpad.i64()
  ret i64 %out
}

declare i64 @llvm.callbr.landingpad.i64()
nickdesaulniers planned changes to this revision.Dec 15 2022, 5:17 PM
nickdesaulniers marked an inline comment as done.
  • langref, remove Builder::SetInsertPoint call, add verifification test
void added a comment.Dec 21 2022, 3:09 PM

As we talked before, I *really* hate the use of an intrinsic here. We used an intrinsic for the old exception handling and it was nothing but buggy. Theoretically they work just fine, but the can't take into account issues due to inlining, block splitting, and other code motion changes (at least not without a hefty dose of special checks throughout the compiler). If we use an intrinsic here, I believe we're just going to revisit the errors of the past.

nickdesaulniers added a comment.EditedDec 21 2022, 3:28 PM

As we talked before, I *really* hate the use of an intrinsic here. We used an intrinsic for the old exception handling and it was nothing but buggy. Theoretically they work just fine, but the can't take into account issues due to inlining, block splitting, and other code motion changes (at least not without a hefty dose of special checks throughout the compiler). If we use an intrinsic here, I believe we're just going to revisit the errors of the past.

The use of an instruction was tried here: https://reviews.llvm.org/D139565.

Specifically, please see the feedback:

While I agree with your points about intrinsic vs instruction; with the approach of the series (as mentioned in the RFC), I'll note that the intrinsic doesn't live long enough to be relevant to inlining, block splitting, or other code motion changes. It is produced by callbrprepare shortly before being consumed by selectiondag.

Please see the pipeline test changes in https://reviews.llvm.org/D140180 to get a sense of the pass "distance" between Prepare callbr (which introduces these intrinsics) and <arch> DAG->DAG Pattern Instruction Selection which consumes them.

I need @void and @efriedma (and me and maybe @jyknight ) to be in agreement as the feedback around instruction vs intrinsic is giving me whiplash in code review. It's up to you two to agree; I can't have @void reject intrinsics while @efriedma rejects instructions.

void added a comment.Jan 3 2023, 3:33 PM

As we talked before, I *really* hate the use of an intrinsic here. We used an intrinsic for the old exception handling and it was nothing but buggy. Theoretically they work just fine, but the can't take into account issues due to inlining, block splitting, and other code motion changes (at least not without a hefty dose of special checks throughout the compiler). If we use an intrinsic here, I believe we're just going to revisit the errors of the past.

The use of an instruction was tried here: https://reviews.llvm.org/D139565.

Specifically, please see the feedback:

While I agree with your points about intrinsic vs instruction; with the approach of the series (as mentioned in the RFC), I'll note that the intrinsic doesn't live long enough to be relevant to inlining, block splitting, or other code motion changes. It is produced by callbrprepare shortly before being consumed by selectiondag.

Please see the pipeline test changes in https://reviews.llvm.org/D140180 to get a sense of the pass "distance" between Prepare callbr (which introduces these intrinsics) and <arch> DAG->DAG Pattern Instruction Selection which consumes them.

I need @void and @efriedma (and me and maybe @jyknight ) to be in agreement as the feedback around instruction vs intrinsic is giving me whiplash in code review. It's up to you two to agree; I can't have @void reject intrinsics while @efriedma rejects instructions.

Thanks for the information. (And sorry I wasn't involved in those discussions.)

The main issue with the way EH used to be handled was that the intrinsic that held all of the information about what EH values are used, etc., could be moved away from the landing pad (via inlining and other normal code motion). It made associating the intrinsic with the proper invoke instruction(s) more-or-less impossible to get correct without having to jump through a ton of hoops. Creating the landingpad instruction, and forcing it to be the first non-PHI instruction in a BB, basically solved all EH issues that existed at that time.

So, if you can ensure that the intrinsic won't be moved around due to normal passes, or, if it is moved, that it won't matter (i.e., you'll be able to associate it with the callbr when necessary), then I'll withdraw my opposition to using an intrinsic.

The use of an IR pass here is basically just a hack to work around the fact that SelectionDAG doesn't have a reasonable way to do this lowering inside SelectionDAG itself. If we were using GlobalISel exclusively, we would probably just implement this as a GlobalISel pass instead of an IR pass.

If we need some additional infrastructure to make the IR pass work reliably, we can look at that, but implementing an IR instruction that only exists for a couple passes inside codegen seems like overkill.

efriedma added inline comments.Jan 3 2023, 4:29 PM
llvm/docs/LangRef.rst
13714 ↗(On Diff #484635)

I'm not sure we need to document this at all (it's an unstable implementation detail users of LLVM shouldn't need to be aware of). But if you do think it's necessary to document it, please call out that it's an unstable implementation detail, and users should not call it.

void added a comment.Jan 4 2023, 3:41 AM

The use of an IR pass here is basically just a hack to work around the fact that SelectionDAG doesn't have a reasonable way to do this lowering inside SelectionDAG itself. If we were using GlobalISel exclusively, we would probably just implement this as a GlobalISel pass instead of an IR pass.

If we need some additional infrastructure to make the IR pass work reliably, we can look at that, but implementing an IR instruction that only exists for a couple passes inside codegen seems like overkill.

Thanks. Okay, I agree with @efriedma and you that an intrinsic should work just fine here. It seems like the lifespan of this intrinsic is going to be very short, so much so that few if any IR changes will occur.

nickdesaulniers marked an inline comment as done.
  • rebase, format, remove intrinsic docs as per @efriedma
This revision is now accepted and ready to land.Jan 18 2023, 11:39 AM
void added inline comments.Jan 18 2023, 3:44 PM
llvm/lib/IR/Verifier.cpp
5776

Please add a break; at the end of this case.

nickdesaulniers planned changes to this revision.Jan 19 2023, 9:47 AM
  • add missing break statement, as per @void
This revision is now accepted and ready to land.Jan 19 2023, 9:59 AM
nickdesaulniers marked an inline comment as done.Jan 19 2023, 9:59 AM
jyknight added inline comments.Jan 19 2023, 11:53 AM
llvm/lib/CodeGen/CallBrPrepare.cpp
124

Doesn't seem like much point in having this a separate variable, can just pass directly to CreateIntrinsic. And I'd merge this fn into InsertIntrinsicCalls -- with construction of Builder outside the loop, and SetInsertPoint+CreateIntrinsic in the loop.

llvm/test/Transforms/SimplifyCFG/callbr-destinations.ll
62

In the expected pass pipeline, do we simplifycfg after callbrprepare? That seems a bit surprising if so?

(Not that it's wrong to mark it IntrNoMerge, of course.)

nickdesaulniers marked an inline comment as done.
  • inline InsertIntrinsicCall, as per @jyknight
llvm/test/Transforms/SimplifyCFG/callbr-destinations.ll
62

We do not; this is an artifact from when the frontend would produce the intrinsic at which point we would need to guard against SimplifyCFG merging two basic blocks that just contained this intrinsic call.

I can remove it if you feel that it's unnecessary to retain this guard. WDYT?

  • rebase (add header I no longer needed to add earlier but do now)
jyknight accepted this revision.Jan 27 2023, 2:48 PM
jyknight added inline comments.
llvm/test/Transforms/SimplifyCFG/callbr-destinations.ll
62

Oh I see. No, fine as is.

nickdesaulniers marked 2 inline comments as done.Jan 30 2023, 2:30 PM