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.
Details
Diff Detail
Unit Tests
Event Timeline
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.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp | ||
---|---|---|
824 | Could MSSAU be created unconditionally like with the legacy pass manager? |
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. |
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? |
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).
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 |
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.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp | ||
---|---|---|
885 | This doesn't look right ... won't this crash if getAnalysisIfAvailable() returns nullptr? |
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. |
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!
Could MSSAU be created unconditionally like with the legacy pass manager?