This is an archive of the discontinued LLVM Phabricator instance.

[LoopFlatten] Update MemorySSA state
ClosedPublic

Authored by SjoerdMeijer on Jan 5 2022, 6:00 AM.

Details

Summary

I would like to move LoopFlatten from LoopPass Manager LPM2 to LPM1 (in D116612), but that is a LPM that is using MemorySSA and so LoopFlatten needs to preserve MemorySSA and this adds that. More specifically, LoopFlatten restructures the CFG and with this change the MSSA state is updated accordingly, where we also update the DomTree. LoopFlatten doesn't rewrite/optimise/delete load or store instructions, so I have not added any MSSA updates for that.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Jan 5 2022, 6:00 AM
SjoerdMeijer requested review of this revision.Jan 5 2022, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 6:00 AM

Ah, forgot to mention tests. Without the update MSSAU->removeEdge(), I was expecting AR.MSSA->verifyMemorySSA() to run in an assert (with the expensive checks enabled in my build), but somewhat surprisingly to me it didn't.
So not sure yet if we need tests for this, but am looking into this.

fhahn added a subscriber: fhahn.Jan 5 2022, 6:08 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

Could MSSAU be created unconditionally like with the legacy pass manager?

SjoerdMeijer added inline comments.Jan 5 2022, 6:26 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

Thanks for taking a look.

I borrowed this boilerplate code from SimpleLoopUnswitch, but sure, I guess so and will add that.

SjoerdMeijer added inline comments.Jan 5 2022, 6:34 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

Hmm, I see this pattern used in most passes, like LoopRotation, LoopSimplifyCFG (SimpleLoopUnswitch).

For my understanding, what would be the benefit of doing this unconditionally?

fhahn added inline comments.Jan 5 2022, 6:40 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

slightly less & more straight-forward code?

slightly less & more straight-forward code?

Fair enough. :-) Was wondering if there was more to it.
Now getting MSSAU unconditionally.

I somehow missed the failing regression test.
Turns out I do need to check AR.MSSA for passing on the MSSA object (or a nullptr).

aeubanks added inline comments.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

with the legacy PM it looks like we're unconditionally requesting MSSA (constructing it if it doesn't yet exist) and then updating it, which is wasted work if MSSA wasn't already available. we only want to keep MSSA up to date if it's already cached

nikic added inline comments.Jan 13 2022, 12:13 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
885

Shouldn't this be using getAnalysisIfAvailable? MSSA is not a required analysis here. Also, getAnalysisUsage() should be adjusted to mark MSSA as preserved.

Thanks for the review, sorry for the delay.
Addressed comments and have added that to the LPM.

nikic added inline comments.Jan 18 2022, 3:15 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
885

This doesn't look right ... won't this crash if getAnalysisIfAvailable() returns nullptr?

Sorry, my bad, should be fixed now.

asbirlea accepted this revision.Jan 18 2022, 2:20 PM

Changes LGTM.
Nit: clang-format.
A simpler way to check all verification is now to pass VerificationLevel::Full to verifyMemorySSA(); I'm mentioning this for your local testing, in case your build doesn't have EXPENSIVE_CHECKS on already, not to include in this patch.

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
824

Nit: I'd stick with the Optional<MemorySSAUpdater> MSSAU; to avoid the object allocation when MSSA is not present.

This revision is now accepted and ready to land.Jan 18 2022, 2:20 PM

Changes LGTM.
Nit: clang-format.
A simpler way to check all verification is now to pass VerificationLevel::Full to verifyMemorySSA(); I'm mentioning this for your local testing, in case your build doesn't have EXPENSIVE_CHECKS on already, not to include in this patch.

I have applied clang-format, double checked locally VerificationLevel::Full (but was using an expensive checks build), and restored optionally allocating MSSAU.

Thanks for reviewing!

This revision was landed with ongoing or failed builds.Jan 19 2022, 2:58 AM
This revision was automatically updated to reflect the committed changes.