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

[nickdesaulniers]

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.

[nickdesaulniers]

llvm/lib/CodeGen/CallBrPrepare.cpp

94

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

[nickdesaulniers]

• insert intrinsic call regardless of critical edge splitting

[nickdesaulniers]

• move insertion of intrinsic calls out of SplitCriticalEdges (prefer small readable functions)

[nickdesaulniers]

• s/Synth/Dest

[nickdesaulniers]

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...

[nickdesaulniers]

• add IntrNoMerge

[aeubanks]

langref?

llvm/lib/CodeGen/CallBrPrepare.cpp

115

can pass this directly to the constructor

[nickdesaulniers]

llvm/lib/IR/Verifier.cpp

5856–5887

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]

• langref, remove Builder::SetInsertPoint call, add verifification test

[nickdesaulniers]

Ready for review:

1. https://reviews.llvm.org/D135997
2. https://reviews.llvm.org/D140166
3. https://reviews.llvm.org/D139861
4. https://reviews.llvm.org/D139872
5. https://reviews.llvm.org/D139883
6. https://reviews.llvm.org/D139970
7. https://reviews.llvm.org/D140160
8. https://reviews.llvm.org/D137113
9. https://reviews.llvm.org/D136497

[void]

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]

In D139883#4011907, @void wrote:

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:

• https://reviews.llvm.org/D139565#3979289
• https://reviews.llvm.org/D139565#3982939

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]

In D139883#4012018, @nickdesaulniers wrote:

In D139883#4011907, @void wrote:

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:

• https://reviews.llvm.org/D139565#3979289
• https://reviews.llvm.org/D139565#3982939

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.

[efriedma]

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]

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]

In D139883#4024498, @efriedma wrote:

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]

• rebase, format, remove intrinsic docs as per @efriedma

[nickdesaulniers]

• rebase

[efriedma]

LGTM

[void]

llvm/lib/IR/Verifier.cpp

5852

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

[nickdesaulniers]

• add missing break statement, as per @void

[jyknight]

llvm/lib/CodeGen/CallBrPrepare.cpp

113

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]

• 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?

[nickdesaulniers]

• rebase (add header I no longer needed to add earlier but do now)

[jyknight]

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

62

Oh I see. No, fine as is.

[nickdesaulniers]

• rebase

[nickdesaulniers]

• merge D142940, final rebase