[PATCH] D127962: [WinEHPrepare] Propagate funclet tokens when inlining into EH funclets

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks, sorry for the delay.



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2313-2324
+          // When targeting WinEH, we must propagate funclet tokens to pre-ISel
+          // intrinsics, if they get inlined into EH funclets. Otherwise,
+          // WinEHPrepare would mark them unreachable and cause truncations.
+          bool PropagateExplicitly = false;
+          if (Personality == EHPersonality::MSVC_CXX) {
+            using namespace llvm::objcarc;
+            ARCInstKind Kind = GetFunctionClass(CalledFn);
----------------
sgraenitz wrote:
> We could simplify the condition here by adding a new helper function in ObjCARCInstKind.h/.cpp:
> ```
> bool llvm::objcarc::IsPreISelIntrinsic(Function *F) {
>   ARCInstKind K = GetFunctionClass(F);
>   return !IsUser(K) && K != ARCInstKind::None;
> }
> ```
> 
> It could be used in D124762 CGCall.cpp as well as in PreISelIntrinsicLowering.cpp to assert the set of pre-ISel intrinsics is in sync with what we do here. This would also connect the three pieces of code explicitly, so it's easier for others to see their relation.
I think the generic predicate we are looking should be called something like `IntrinsicInst::mayLowerToFunctionCall`, and it would be true for these ObjC ARC intrinsics, statepoint, and anything else like that. Basically, we should apply funclet operand bundles to intrinsics that lower to function calls.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2317
+          bool PropagateExplicitly = false;
+          if (Personality == EHPersonality::MSVC_CXX) {
+            using namespace llvm::objcarc;
----------------
The personality check should not be necessary. We check CallSiteEHPad above, and that's enough to imply that the current function uses funclet bundle operands. (Wasm, SEH, C++ EH)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127962