[PATCH] D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 3 inline comments as done.
chandlerc added a comment.

Thanks Davide, I think I've got these covered either by adjustments or future work. Give a shout about anything you'd still like addresed in follow-up patches.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1139
+  auto OrderedClonedExitsInLoops = ClonedExitsInLoops;
+  std::stable_sort(OrderedClonedExitsInLoops.begin(),
+                   OrderedClonedExitsInLoops.end(),
----------------
davide wrote:
> Can you add a comment here explaining with a non-stable sort is not enough?
At this point, I don't think it actually matters because I ended up using a stable order below, so I've changed this. Thanks!


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1245
+      // update the domtree.
+      assert((L.contains(ChildN->getBlock()) ||
+              llvm::find(ExitBlocks, ChildN->getBlock()) != ExitBlocks.end()) &&
----------------
davide wrote:
> 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.
Yep. I've got that in the change description as one of the obvious follow-ups.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1809
+#endif
+    formLCSSA(UpdateL, DT, &LI, nullptr);
+  };
----------------
davide wrote:
> 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.
Yep. Sadly, I wasn't able to easily come up with a better way, but I can believe there is one.

This at least doesn't do the recursive formulation...


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1858
+  // progress in the pass pipeline.
+  LI.verify(DT);
+#endif
----------------
davide wrote:
> Want to add also `verifyisRecursivelyLCSSA` here? or whatever is called?
We already verify all child loops of updated loops in UpdateLCSSA. So the only way this would help is if the LCSSA formation itself is busted.


https://reviews.llvm.org/D34200