This is an archive of the discontinued LLVM Phabricator instance.

[FunctionPropertiesAnalysis] Generalize support for unreachable
ClosedPublic

Authored by mtrofin on Jun 15 2022, 5:00 PM.

Details

Summary

Generalized support for subgraphs that get rendered unreachable, for
both call and invoke cases.

Diff Detail

Event Timeline

mtrofin created this revision.Jun 15 2022, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 5:00 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
mtrofin requested review of this revision.Jun 15 2022, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 5:00 PM
kazu added a comment.Jun 17 2022, 1:09 PM

LGTM modulo cosmetic changes. Your new algorithm looks pretty robust to me. Thanks!

llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
156–160

An instruction can never be CallInst and InvokeInst at the same time because InvokeInst and CallInst are siblings derived from CallBase. Yo could shave one line with:

if (const auto *II = dyn_cast<InvokeInst>(&CB)) {
  const auto *UnwindDest = II->getUnwindDest();
  Successors.insert(succ_begin(UnwindDest), succ_end(UnwindDest));
}
230–232

Could we avoid the negated name for clarity? We can shorten the variable name and remove one line.

if (I >= IncludeSuccessorsMark)
  Reinclude.insert(succ_begin(BB), succ_end(BB));
233

May I suggest an empty line immediately after the loop? We are starting a new code block here.

kazu accepted this revision.Jun 17 2022, 1:09 PM
This revision is now accepted and ready to land.Jun 17 2022, 1:09 PM
mtrofin updated this revision to Diff 438707.Jun 21 2022, 8:02 AM
mtrofin marked 3 inline comments as done.

feedback

This revision was landed with ongoing or failed builds.Jun 21 2022, 8:18 AM
This revision was automatically updated to reflect the committed changes.