ahatanak added inline comments.
================
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();
----------------
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?
================
Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+ DenseMap<BasicBlock *, ColorVector> 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
