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

Tue Jan 24 02:28:12 PST 2023

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<FuncletPadInst>(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 &CV = BlockColors.find(&BB)->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