This is an archive of the discontinued LLVM Phabricator instance.

[ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens
ClosedPublic

Authored by sgraenitz on Nov 14 2022, 6:34 AM.

Details

Summary

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.

Diff Detail

Event Timeline

sgraenitz created this revision.Nov 14 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 6:34 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sgraenitz requested review of this revision.Nov 14 2022, 6:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 14 2022, 6:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

rnk added a comment.Nov 18 2022, 11:57 AM

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

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
767

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.

2040–2041

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.

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

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
767

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 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.

2040–2041

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).

sgraenitz updated this revision to Diff 477470.Nov 23 2022, 5:09 AM
sgraenitz marked an inline comment as done.

Rebase and update check-lines after D137939

sgraenitz updated this revision to Diff 477471.Nov 23 2022, 5:12 AM

Handle potential multi-color basic blocks in addOpBundleForFunclet()

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

ahatanak added inline comments.Dec 9 2022, 11:55 AM
llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
767

Is the assertion assert(CV.size() == 1 && "non-unique color for block!"); in CloneCallInstForBB incorrect too?

2040–2041

OptimizeIndividualCall calls colorEHFunclets too. Is it possible to just call it once? Or we don't care because it's not expensive?

sgraenitz updated this revision to Diff 483745.Dec 17 2022, 5:24 AM
sgraenitz marked 2 inline comments as done.

Address feedback

sgraenitz added a comment.EditedDec 17 2022, 5:41 AM

Thanks for taking a look. Yes, the coloring can be expensive and we shouldn't run it more than once. OptimizeIndividualCalls did it and actually even OptimizeSequences runs in a loop "until it either stops making changes or no retain+release pair nesting is detected". I completely missed that.

I moved funclet coloring into ObjCARCOpt::init() so that it runs once for the whole pass now. The results are cached in ObjCARCOpt::BlockEHColors. addOpBundleForFunclet() and cloneCallInstForBB became member functions that can directly use the cached results. We don't need to pass them as parameters from function to function anymore. I think it's not only more efficient now but also much cleaner. That was really useful feedback @ahatanak! Thanks

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
767

The code path disappears with the subsequent patch D137945. But yes, it's formally incorrect and I adjusted the assertion in the last iteration.

2040–2041

Good catch! Right, we shouldn't do it multiple times. Having moved funclet coloring into ObjCARCOpt::init() it now runs only once.

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.

ahatanak accepted this revision.Jan 23 2023, 6:41 AM

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

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
598

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?

This revision is now accepted and ready to land.Jan 23 2023, 6:41 AM
sgraenitz marked an inline comment as done.Jan 24 2023, 2:28 AM
sgraenitz added inline comments.
llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
598

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

751

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

This revision was landed with ongoing or failed builds.Jan 24 2023, 6:17 AM
This revision was automatically updated to reflect the committed changes.
sgraenitz marked an inline comment as done.