This is an archive of the discontinued LLVM Phabricator instance.

[PruneEH][NewPM] Port prune-eh to NPM
AbandonedPublic

Authored by aeubanks on Sep 14 2020, 12:09 PM.

Details

Reviewers
asbirlea
ychen
Summary

All tests under Transforms/PruneEH/ now pass under NPM.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 14 2020, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2020, 12:09 PM
aeubanks requested review of this revision.Sep 14 2020, 12:09 PM
aeubanks updated this revision to Diff 291641.

remove debugging code

aeubanks updated this revision to Diff 291643.Sep 14 2020, 12:12 PM

extra parentheses

aeubanks retitled this revision from [PrunEH][NewPM] Port prune-eh to NPM to [PruneEH][NewPM] Port prune-eh to NPM.Sep 14 2020, 12:16 PM

I think for clarify this could be split into a patch that updates the current pass to use the CallGraphUpdater, and a patch that only adds the pass for the new pass manager.

llvm/lib/Transforms/IPO/PruneEH.cpp
75–76

I'm not clear if at this point we guarantee that there are no nullptrs in Functions. It would be good to pre-check this.
e.g.

if (Functions.contains (nullptr)) {
  SCCMightUnwind = true;
  SCCMightReturn = true;
  Functions.delete(nullptr); //then the for here and at line 158 doesn't need the check which was in the original and removed here.
}
85–86

This check for null won't be needed then.

86–87

The early exit condition !SCCMightUnwind || !SCCMightReturn could be triggered here, so perhaps add:

if (SCCMightUnwind && SCCMightReturn)
          break;

I think for clarify this could be split into a patch that updates the current pass to use the CallGraphUpdater, and a patch that only adds the pass for the new pass manager.

CallGraphUpdater: https://reviews.llvm.org/D87632
There I also added asserts that all Functions were not null.

aeubanks updated this revision to Diff 291690.Sep 14 2020, 2:41 PM

Move break outside if/else

aeubanks updated this revision to Diff 291696.Sep 14 2020, 2:48 PM

rebase past CallGraphUpdater change

aeubanks edited the summary of this revision. (Show Details)Sep 14 2020, 2:48 PM

asbirlea, you mentioned offline that there were passes that replaced this in the NPM and that this shouldn't be ported, do you know what those are so I can put them in the description of another change pinning tests to the legacy PM?

I think for prune-eh, the NPM inliner was intended to address the same cleanup, but I don't have all the context.
@chandlerc for more details.

Looking at the pass, it looks like it can be run independently of an inliner since it simply changes invokes into calls whenever possible.
There are lots of random tests that use this pass, and there's even a C API for it.
I think we should port this, and this patch is pretty small. Of course we can always remove it later if we decide to.