[PATCH] D93764: [LoopUnswitch] Implement first version of partial unswitching.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.





================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:671
+    if (!I || L->isLoopInvariant(I))
+      continue;
+
----------------
jdoerfert wrote:
> Does code generation handle the case where a loop invariant instruction is inside the loop? With the comment below, this might be changed to `L->contains(I)` instead.
Loop-invariant instructions will be duplicated outside the loop at the moment (added a test). With 'handling them', do you mean updating the uses inside the loop to use the hoisted instruction? Not sure if that will help a lot in practice, as they should already have been moved out I think and/or GVN/LICM will clean them up.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:674
+    if (!isa<LoadInst>(I) && !isa<GetElementPtrInst>(I))
+      return {};
+
----------------
jdoerfert wrote:
> "Later" we could allow anything that is speculatively executable and loads, right?
> 
yep I think so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764