[PATCH] D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple.
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 13 15:48:04 PST 2017
davide added a comment.
I have few comments. As Sanjoy already approved, feel free to update this and check in without another round trip.
Most of them can be addressed in follow-ups, but I thought they're worth mentioning regardless. Thank you for pushing forward this.
It's been a long time coming :)
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1139
+ auto OrderedClonedExitsInLoops = ClonedExitsInLoops;
+ std::stable_sort(OrderedClonedExitsInLoops.begin(),
+ OrderedClonedExitsInLoops.end(),
----------------
Can you add a comment here explaining with a non-stable sort is not enough?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1245
+ // update the domtree.
+ assert((L.contains(ChildN->getBlock()) ||
+ llvm::find(ExitBlocks, ChildN->getBlock()) != ExitBlocks.end()) &&
----------------
This doesn't (yet) use the new dominator incremental update API, from what I can tell.
I think it can be done in a separate change, but I think it's worth doing instead of rolling its own update logic.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1789
+ // the nature of the change) to more efficiently update things.
+ DT.recalculate(*SplitBB->getParent());
+
----------------
same here.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1809
+#endif
+ formLCSSA(UpdateL, DT, &LI, nullptr);
+ };
----------------
JFYI, rebuilding LCSSA in LLVM is currently a sledgehammer for some use cases.
It's of course, not your fault, as the algorithm is O(N^3) [I can provide cases that are tight :)], but let's be prepared to fix it in case this turns out to be a compile time sink.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1858
+ // progress in the pass pipeline.
+ LI.verify(DT);
+#endif
----------------
Want to add also `verifyisRecursivelyLCSSA` here? or whatever is called?
https://reviews.llvm.org/D34200
More information about the llvm-commits
mailing list