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 &CV = 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<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. ================ Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041 + DenseMap<BasicBlock *, ColorVector> 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