This is an archive of the discontinued LLVM Phabricator instance.

[HardwareLoops] NewPM support
ClosedPublic

Authored by samparker on Jan 4 2023, 6:29 AM.

Details

Summary

We're now defaulting to preserving LCSSA, with the NPM, so a couple of tests have changed slightly.

Diff Detail

Event Timeline

samparker created this revision.Jan 4 2023, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 6:29 AM
samparker requested review of this revision.Jan 4 2023, 6:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 4 2023, 6:29 AM
shchenz added inline comments.Jan 5 2023, 3:05 AM
llvm/lib/CodeGen/HardwareLoops.cpp
287

Seems this is an issue from LPM, but hardware loop insertion pass should preserve some other analysis as well, like ScalarEvolutionAnalysis, BranchProbabilityAnalysis, DependenceAnalysis and MemorySSAAnalysis (although there is no handling for it now, we may need pass it to InsertPreheaderForLoop if want to preserve this analysis)? I saw some other pass that calls InsertPreheaderForLoop but declare to preserve these analysis.

samparker added inline comments.Jan 12 2023, 2:01 AM
llvm/lib/CodeGen/HardwareLoops.cpp
287

I can't say I understand the LPM, but we're not using it here, nor are we using MemorySSA or BranchProbability. So could you please explain why/what you'd like to see change here?

shchenz added inline comments.Jan 12 2023, 7:19 PM
llvm/lib/CodeGen/HardwareLoops.cpp
287

I think the pass should try its best to declare the preservation of analysis passes to avoid the analysis passes are run again unnecessarily. If we don't change BranchProbability or some other analysis pass results, we should better to declare it as preserved, otherwise later pass that depends on this analysis will have to rerun the analysis pass again? I think this is the reason why we return PreservedAnalyses::all() if the pass does not do any transformation.

I said the LPM because in the legacy pass implementation of hardware loop pass, it also preserves these two analysis DominatorTreeAnalysis and LoopAnalysis, so I thought you were implementing the same thing in NPM with the LPM?

samparker added inline comments.Jan 12 2023, 9:15 PM
llvm/lib/CodeGen/HardwareLoops.cpp
287

Ah, sorry, for some reason I've always thought we can only 'preserve' anything that we directly use!

As for the LPM, I guess my ignorance continues... I don't think I'm using it? I thought this is just a port of a function pass into another function pass.

samparker updated this revision to Diff 488862.Jan 12 2023, 9:46 PM

Added extra preserved analyses, though omitting MemorySSA for now.

shchenz accepted this revision.Feb 12 2023, 6:45 PM

LGTM.

This revision is now accepted and ready to land.Feb 12 2023, 6:45 PM
This revision was landed with ongoing or failed builds.Feb 13 2023, 1:47 AM
This revision was automatically updated to reflect the committed changes.

@samparker Good morning/afternoon from the UK :)

I believe this patch may have caused this following build bot to start failing:

https://lab.llvm.org/buildbot/#/builders/168/builds/11878

The test is CodeGen/ARM/O3-pileline.ll

Are you able to take a look at it?

much appreciated.

Thank you so much, this has been fixed mid-investigation, apologies for the unnecessary ping.