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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Overall, I think this is the right place for such logic. Some initial code comments.

TBH, I think it would be nicer to "search for the paths" instead. Though, a TODO might be sufficient for the start. 
What I think would be nice if we do not only deal with the conditional in the header, still thinking of `*l && *r`.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:669
+    Instruction *I = dyn_cast<Instruction>(WorkList.back());
+    WorkList.pop_back();
+    if (!I || L->isLoopInvariant(I))
----------------
`.pop_back_val()` ?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:671
+    if (!I || L->isLoopInvariant(I))
+      continue;
+
----------------
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.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764