[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 &CV = 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<FuncletPadInst>(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<BasicBlock *, ColorVector> 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