[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sgraenitz marked an inline comment as done.
Closed by commit rGd9eece916a8a: [ObjC][ARC] Teach the OptimizeSequences step 
of ObjCARCOpts about WinEH funclet… (authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -501,6 +501,8 @@
   /// is in fact used in the current function.
   unsigned UsedInThisFunction;
 
+  DenseMap BlockEHColors;
+
   bool OptimizeRetainRVCall(Function , Instruction *RetainRV);
   void OptimizeAutoreleaseRVCall(Function , Instruction *AutoreleaseRV,
  ARCInstKind );
@@ -508,17 +510,16 @@
 
   /// Optimize an individual call, optionally passing the
   /// GetArgRCIdentityRoot if it has already been computed.
-  void OptimizeIndividualCallImpl(
-  Function , DenseMap ,
-  Instruction *Inst, ARCInstKind Class, const Value *Arg);
+  void OptimizeIndividualCallImpl(Function , Instruction *Inst,
+  ARCInstKind Class, const Value *Arg);
 
   /// Try to optimize an AutoreleaseRV with a RetainRV or UnsafeClaimRV.  If the
   /// optimization occurs, returns true to indicate that the caller should
   /// assume the instructions are dead.
-  bool OptimizeInlinedAutoreleaseRVCall(
-  Function , DenseMap ,
-  Instruction *Inst, const Value *, ARCInstKind Class,
-  Instruction *AutoreleaseRV, const Value *);
+  bool OptimizeInlinedAutoreleaseRVCall(Function , Instruction *Inst,
+const Value *, ARCInstKind Class,
+Instruction *AutoreleaseRV,
+const Value *);
 
   void CheckForCFGHazards(const BasicBlock *BB,
   DenseMap ,
@@ -566,12 +567,46 @@
 
   void OptimizeReturns(Function );
 
+  Instruction *cloneCallInstForBB(CallInst , BasicBlock ) {
+SmallVector OpBundles;
+for (unsigned I = 0, E = CI.getNumOperandBundles(); I != E; ++I) {
+  auto Bundle = CI.getOperandBundleAt(I);
+  // Funclets will be reassociated in the future.
+  if (Bundle.getTagID() == LLVMContext::OB_funclet)
+continue;
+  OpBundles.emplace_back(Bundle);
+}
+
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find()->second;
+  assert(CV.size() > 0 && "non-unique color for block!");
+  Instruction *EHPad = CV.front()->getFirstNonPHI();
+  if (EHPad->isEHPad())
+OpBundles.emplace_back("funclet", EHPad);
+}
+
+return CallInst::Create(, OpBundles);
+  }
+
+  void addOpBundleForFunclet(BasicBlock *BB,
+ SmallVectorImpl ) {
+if (!BlockEHColors.empty()) {
+  const ColorVector  = 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz marked an inline comment as done.
sgraenitz added inline comments.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:597
+  assert(CV.size() > 0 && "Uncolored block");
+  for (BasicBlock *EHPadBB : CV)
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI())) 
{

ahatanak wrote:
> This piece of code does something similar to `cloneCallInstForBB`, but it's 
> slightly different from it. It iterates over the entries in `CV` whereas the 
> code above just looks at the first entry. 
> 
> Is this a bug that is fixed in https://reviews.llvm.org/D137945?
Yes, D137945 unifies the behavior so that we will always iterate. The unique 
color invariant only holds after WinEHPrepare. So the old assertion in 
`cloneCallInstForBB()` was formally wrong, but I only ever observed cases of 
non-unique coloring in incorrect IR, i.e. dangling funclet tokens before 
D137939. Iteration is the safe solution, especially since we may not be able to 
bail out on dangling funclet tokens in the verifier (see 
https://reviews.llvm.org/D138123#4067201).

FYI: We discussed it this in a previous round of this review 
https://reviews.llvm.org/D137944?id=475132#inline-1334823



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:751
-const ColorVector  = BlockColors.find()->second;
-assert(CV.size() == 1 && "non-unique color for block!");
-Instruction *EHPad = CV.front()->getFirstNonPHI();

The old code here assumed all EH coloring to be unique.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak accepted this revision.
ahatanak added a comment.
This revision is now accepted and ready to land.

I'm not familiar with the WinEH stuff, but the other parts (the code that adds 
bundles, etc.) LGTM.




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:597
+  assert(CV.size() > 0 && "Uncolored block");
+  for (BasicBlock *EHPadBB : CV)
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI())) 
{

This piece of code does something similar to `cloneCallInstForBB`, but it's 
slightly different from it. It iterates over the entries in `CV` whereas the 
code above just looks at the first entry. 

Is this a bug that is fixed in https://reviews.llvm.org/D137945?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This review belongs to a series of patches. I am planning to land  it towards 
the end of next week, so that it's ready for the next release branching in 
February. If you have any more remarks, please let me know soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for taking a look. That' really useful feedback! Yes, the coloring can 
be expensive and we shouldn't run it more than once.

and there's more places indeed

until it either stops making changes or

  // no retain+release pair nesting is detected




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

ahatanak wrote:
> sgraenitz wrote:
> > rnk wrote:
> > > I don't think the verifier changes you made guarantee this. I would 
> > > consider strengthening this to `report_fatal_error`, since valid IR 
> > > transforms can probably reach this code.
> > Right, I guess the Verifier change is correct and this should change to 
> > work for multi-color BBs?
> > ```
> >   assert(CV.size() > 0 && "Uncolored block");
> >   for (BasicBlock *EHPadBB : CV)
> > if (auto *EHPad = 
> > dyn_cast(ColorFirstBB->getFirstNonPHI())) {
> >   OpBundles.emplace_back("funclet", EHPad);
> >   break;
> > }
> > ```
> > 
> > There is no test that covers this case, but it appears that the unique 
> > color invariant only holds **after** WinEHPrepare. [The assertion 
> > here](https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/CodeGen/WinEHPrepare.cpp#L185)
> >  seems to imply it:
> > ```
> > assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");
> > ```
> > 
> > Since it would be equivalent to the Verifier check, I'd stick with the 
> > assertion and not report a fatal error.
> Is the assertion `assert(CV.size() == 1 && "non-unique color for block!");` 
> in `CloneCallInstForBB` incorrect too?
The code path disappears with the subsequent patch D137945. But yes, it's 
formally incorrect and I adjusted the assertion in the last iteration.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

ahatanak wrote:
> sgraenitz wrote:
> > rnk wrote:
> > > I think this code snippet probably deserves a Function helper, 
> > > `F->hasScopedEHPersonality()`. Another part of me thinks we should cache 
> > > the personality enum similar to the way we cache intrinsic ids, but I 
> > > wouldn't want to increase Function memory usage, so that seems premature. 
> > > "No action required", I guess.
> > It doesn't really fit the the scope of this patch but I am happy to add the 
> > helper function in a follow-up (for now without personality enum caching).
> `OptimizeIndividualCall` calls `colorEHFunclets` too. Is it possible to just 
> call it once? Or we don't care because it's not expensive?
Good catch! Right, we shouldn't do it multiple times. Having moved funclet 
coloring into `ObjCARCOpt::init()` it now runs only once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 483745.
sgraenitz marked 2 inline comments as done.
sgraenitz added a comment.

Address feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -502,6 +502,8 @@
   /// is in fact used in the current function.
   unsigned UsedInThisFunction;
 
+  DenseMap BlockEHColors;
+
   bool OptimizeRetainRVCall(Function , Instruction *RetainRV);
   void OptimizeAutoreleaseRVCall(Function , Instruction *AutoreleaseRV,
  ARCInstKind );
@@ -509,17 +511,16 @@
 
   /// Optimize an individual call, optionally passing the
   /// GetArgRCIdentityRoot if it has already been computed.
-  void OptimizeIndividualCallImpl(
-  Function , DenseMap ,
-  Instruction *Inst, ARCInstKind Class, const Value *Arg);
+  void OptimizeIndividualCallImpl(Function , Instruction *Inst,
+  ARCInstKind Class, const Value *Arg);
 
   /// Try to optimize an AutoreleaseRV with a RetainRV or UnsafeClaimRV.  If the
   /// optimization occurs, returns true to indicate that the caller should
   /// assume the instructions are dead.
-  bool OptimizeInlinedAutoreleaseRVCall(
-  Function , DenseMap ,
-  Instruction *Inst, const Value *, ARCInstKind Class,
-  Instruction *AutoreleaseRV, const Value *);
+  bool OptimizeInlinedAutoreleaseRVCall(Function , Instruction *Inst,
+const Value *, ARCInstKind Class,
+Instruction *AutoreleaseRV,
+const Value *);
 
   void CheckForCFGHazards(const BasicBlock *BB,
   DenseMap ,
@@ -567,12 +568,46 @@
 
   void OptimizeReturns(Function );
 
+  Instruction *cloneCallInstForBB(CallInst , BasicBlock ) {
+SmallVector OpBundles;
+for (unsigned I = 0, E = CI.getNumOperandBundles(); I != E; ++I) {
+  auto Bundle = CI.getOperandBundleAt(I);
+  // Funclets will be reassociated in the future.
+  if (Bundle.getTagID() == LLVMContext::OB_funclet)
+continue;
+  OpBundles.emplace_back(Bundle);
+}
+
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find()->second;
+  assert(CV.size() > 0 && "non-unique color for block!");
+  Instruction *EHPad = CV.front()->getFirstNonPHI();
+  if (EHPad->isEHPad())
+OpBundles.emplace_back("funclet", EHPad);
+}
+
+return CallInst::Create(, OpBundles);
+  }
+
+  void addOpBundleForFunclet(BasicBlock *BB,
+ SmallVectorImpl ) {
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find(BB)->second;
+  assert(CV.size() > 0 && "Uncolored block");
+  for (BasicBlock *EHPadBB : CV)
+if (auto *EHPad = 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

sgraenitz wrote:
> rnk wrote:
> > I don't think the verifier changes you made guarantee this. I would 
> > consider strengthening this to `report_fatal_error`, since valid IR 
> > transforms can probably reach this code.
> Right, I guess the Verifier change is correct and this should change to work 
> for multi-color BBs?
> ```
>   assert(CV.size() > 0 && "Uncolored block");
>   for (BasicBlock *EHPadBB : CV)
> if (auto *EHPad = 
> dyn_cast(ColorFirstBB->getFirstNonPHI())) {
>   OpBundles.emplace_back("funclet", EHPad);
>   break;
> }
> ```
> 
> There is no test that covers this case, but it appears that the unique color 
> invariant only holds **after** WinEHPrepare. [The assertion 
> here](https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/CodeGen/WinEHPrepare.cpp#L185)
>  seems to imply it:
> ```
> assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");
> ```
> 
> Since it would be equivalent to the Verifier check, I'd stick with the 
> assertion and not report a fatal error.
Is the assertion `assert(CV.size() == 1 && "non-unique color for block!");` in 
`CloneCallInstForBB` incorrect too?



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

sgraenitz wrote:
> rnk wrote:
> > I think this code snippet probably deserves a Function helper, 
> > `F->hasScopedEHPersonality()`. Another part of me thinks we should cache 
> > the personality enum similar to the way we cache intrinsic ids, but I 
> > wouldn't want to increase Function memory usage, so that seems premature. 
> > "No action required", I guess.
> It doesn't really fit the the scope of this patch but I am happy to add the 
> helper function in a follow-up (for now without personality enum caching).
`OptimizeIndividualCall` calls `colorEHFunclets` too. Is it possible to just 
call it once? Or we don't care because it's not expensive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This is ready to land and I'd appreciate feedback from one of the authors. 
Anyone else I should add for review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 477471.
sgraenitz added a comment.

Handle potential multi-color basic blocks in addOpBundleForFunclet()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,20 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() > 0 && "Uncolored block");
+for (BasicBlock *EHPadBB : CV)
+  if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI())) {
+OpBundles.emplace_back("funclet", EHPad);
+return;
+  }
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1773,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1789,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
 Call->setDoesNotThrow();
 Call->setTailCall();
 
@@ -1786,7 +1804,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Release);
-CallInst *Call = CallInst::Create(Decl, 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 477470.
sgraenitz marked an inline comment as done.
sgraenitz added a comment.

Rebase and update check-lines after D137939 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,18 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI()))
+  OpBundles.emplace_back("funclet", EHPad);
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1771,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1787,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
 Call->setDoesNotThrow();
 Call->setTailCall();
 
@@ -1786,7 +1802,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Release);
-

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for your feedback @rnk! I hope some of the ObjCARCOpts authors will 
share their opinions as well.




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

rnk wrote:
> I don't think the verifier changes you made guarantee this. I would consider 
> strengthening this to `report_fatal_error`, since valid IR transforms can 
> probably reach this code.
Right, I guess the Verifier change is correct and this should change to work 
for multi-color BBs?
```
  assert(CV.size() > 0 && "Uncolored block");
  for (BasicBlock *EHPadBB : CV)
if (auto *EHPad = 
dyn_cast(ColorFirstBB->getFirstNonPHI())) {
  OpBundles.emplace_back("funclet", EHPad);
  break;
}
```

There is no test that covers this case, but it appears that the unique color 
invariant only holds **after** WinEHPrepare. [The assertion 
here](https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/CodeGen/WinEHPrepare.cpp#L185)
 seems to imply it:
```
assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");
```

Since it would be equivalent to the Verifier check, I'd stick with the 
assertion and not report a fatal error.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

rnk wrote:
> I think this code snippet probably deserves a Function helper, 
> `F->hasScopedEHPersonality()`. Another part of me thinks we should cache the 
> personality enum similar to the way we cache intrinsic ids, but I wouldn't 
> want to increase Function memory usage, so that seems premature. "No action 
> required", I guess.
It doesn't really fit the the scope of this patch but I am happy to add the 
helper function in a follow-up (for now without personality enum caching).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I can't say I'm familiar with ARC, but this seems right to me.




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

I don't think the verifier changes you made guarantee this. I would consider 
strengthening this to `report_fatal_error`, since valid IR transforms can 
probably reach this code.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

I think this code snippet probably deserves a Function helper, 
`F->hasScopedEHPersonality()`. Another part of me thinks we should cache the 
personality enum similar to the way we cache intrinsic ids, but I wouldn't want 
to increase Function memory usage, so that seems premature. "No action 
required", I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

There doesn't seem to be a 1-to-1 relationship between deleted and inserted 
retain/release calls and thus we don't attempt to preserve bundle operands in 
general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

There was a similar effort for CleanupPads in the past: 
https://reviews.llvm.org/rG8b342680bf62722e5099074e8bd23491c71d92b3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: gottesmm, rjmccall, rnk.
Herald added a subscriber: hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

When optimizing retain-release-sequences we insert (and delete) ObjC runtime 
calls. These calls need a funclet operand bundle that refers to the enclosing 
funclet pad whenever they are inserted in a WinEH funclet. WinEH funclets can 
contain multiple basic blocks. In order to find the enclosing funclet pad, we 
have to calculate the funclet coloring first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,18 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI()))
+  OpBundles.emplace_back("funclet", EHPad);
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1771,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1787,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(),