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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:655
+  /// side-effects and no loop value is used outside the loop).
+  bool PathIsNoop = false;
+
----------------
I would have assumed true here and no = true below.


================
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
----------------
Both assignments should not be needed, or assertions.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:820
+          }
+        }
         return true;
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:1094
+        return true;
+      }
     }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95468