[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]

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