[PATCH] D46706: [PM/LoopUnswitch] Support partial trivial unswitching.

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:228
+  if (!SimplifyBlockSet.empty()) {
+    // If we still have blocks to process it is because we have instructions
+    // across multiple blocks. We want to ensure we simplify defs before their
----------------
sanjoy wrote:
> Maybe this can go in a different "loop instruction simplify" pass?  Just above you say "the only real goal of this is to do very basic cleanup of unswitched conditions.  We don't need powerful tools here. A proper pass can be scheduled to do more comprehensive cleanup." :)
> 
> I'm worried that doing all of this in a single pass makes it less granular and harder to test -- we only get to the see the IR after all of this cross-block simplification has happened, which means it is harder to figure out what exactly loop unswitching did.
Second that.
With our current LoopUnswitch use we tend to run massive cleanup after it, and I'm sure we will continue doing that with the new LoopUnswitch. Having a small cleanup in LoopUnswitch itself will not do a full job, and as Sanjoy rightfully points out it might make IR investigations a bit more complicated.



Repository:
  rL LLVM

https://reviews.llvm.org/D46706