[PATCH] D95468: [LoopUnswitch] Add shortcut if unswitched path is a no-op.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:735
+        Info.PathIsNoop = true;
+        Info.ExitForPath = nullptr;
         // First, collect all blocks in the loop that are on a patch from Succ
----------------
jdoerfert wrote:
> Both assignments should not be needed, or assertions.
I changed the code so `IVConditionInfo` is constructed here and returned directly by this function. So we can just start out with the (adjusted) defaults.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:820
+          }
+        }
         return true;
----------------
jdoerfert wrote:
> So the single exit stuff is not actually a conceptual requirement here, is it? I assume you need a single exit for your transformation but you could also collect all exists and later decide to act on them or not, right? Looking at `no_partial_unswitch_shortcut_multi_exit`, it has a single exit actually, it can be reached from multiple exiting blocks but a single exit.
If multiple exit blocks can be reached, we would need to create a new exit block and branch to the correct original exit. Doable, but I am not sure it is worth the effort, at least initially.

Thanks for pointing out the issue in `no_partial_unswitch_shortcut_multi_exit`. This can indeed be handled by the current logic (different exiting blocks branching to the same exit block). I updated the logic above to support this case.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:1094
+        return true;
+      }
     }
----------------
jdoerfert wrote:
> Flip the order here. First the shorter case where we use `unswitchIfProfitable` then the more complex one. The way it is is harder to read.
The main advantage of doing the 'no-op path' handling first is that it avoids full unswitching. If we do `unswitchIfProfitable` first, then we might unnecessarily do full unswitching. Functionally it shouldn't be a big deal, because loop-delete should just remove the loop again, but it skips a bit of unnecessary work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95468/new/

https://reviews.llvm.org/D95468