This is an archive of the discontinued LLVM Phabricator instance.

[LoopReroll][NewPM] Port -loop-reroll to NPM
ClosedPublic

Authored by aeubanks on Sep 18 2020, 5:29 PM.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 18 2020, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 5:29 PM
aeubanks requested review of this revision.Sep 18 2020, 5:29 PM
asbirlea added inline comments.Sep 21 2020, 12:13 PM
llvm/lib/Transforms/Scalar/LoopRerollPass.cpp
1726

Isn't anything preserved? LCSSA and AA perhaps?
AFAICT, getLoopPassPreservedAnalyses() does not apply; DT, LI and ScEv are not preserved.

aeubanks added inline comments.Sep 21 2020, 6:36 PM
llvm/lib/Transforms/Scalar/LoopRerollPass.cpp
1726

Sorry, I didn't notice that the legacy PM's getLoopAnalysisUsage() preserved some analyses.

I'm not super familiar with loop passes, but getLoopPassPreservedAnalyses() says

/// Returns the minimum set of Analyses that all loop passes must preserve.

so why wouldn't that apply here?

asbirlea added inline comments.Sep 23 2020, 12:17 PM
llvm/lib/Transforms/Scalar/LoopRerollPass.cpp
1726

Good point, reading the code it looked like certain changes would affect the CFG and there were no DT or LI updates. Looking further, it looks like only instructions are added/updated, and not the BB, so it looks like we should use getLoopPassPreservedAnalyses here.

For the LPM, the above mustPreserveAnalysisID doesn't make sense to me if the pass already requires LCSSA in the getLoopAnalysisUsage call. It should always be true.

aeubanks updated this revision to Diff 294388.Sep 25 2020, 11:51 AM

preserve loop analyses

asbirlea accepted this revision.Sep 25 2020, 12:05 PM
This revision is now accepted and ready to land.Sep 25 2020, 12:05 PM
This revision was landed with ongoing or failed builds.Sep 25 2020, 12:12 PM
This revision was automatically updated to reflect the committed changes.