[PATCH] D136233: [SimpleLoopUnswitch] Inject loop-invariant conditions and unswitch them when it's profitable

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2963-2966
+  if (auto *MDNode = TI->getMetadata(LLVMContext::MD_make_implicit))
+    NewTerm->setMetadata(LLVMContext::MD_make_implicit, MDNode);
+  if (auto *MDNode = TI->getMetadata(LLVMContext::MD_prof))
+    NewTerm->setMetadata(LLVMContext::MD_prof, MDNode);
----------------
The injected condition doesn't necessarily have the same profile as the original.

I also don't think that we should preserve MD_make_implicit.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3048
       UnswitchCandidates, L, DT, LI, AC, TTI, PartialIVInfo);
-
   assert(Best.TI && "Failed to find loop unswitch candidate");
----------------
Stray change.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3056-3069
+///
+/// We can unswitch by condition "C1 <=u C2". If that is true, then "x <u C1 <=
+/// C2" automatically implies "x <u C2", so we can get rid of one of
+/// loop-variant checks in unswitched loop version.
+/// More generally speaking, we can do the same thing to the following checks:
+///
+///   for (...) {
----------------
Since you are not handling zexts in this patch, this part of the comment can be dropped.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3089-3090
+  DenseMap<Value *, SmallVector<CompareDesc, 4> > CandidatesULT;
+  for (auto *DTN = DT.getNode(Latch); L.contains(DTN->getBlock());
+       DTN = DTN->getIDom()) {
+    ICmpInst::Predicate Pred;
----------------
Why do you limit the transform to conditions that dominate the latch?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3099-3101
+    // Skip branches that have already been unswithed this way.
+    if (Term->hasMetadata("llvm.invariant.condition.injection.disabled"))
+      continue;
----------------
Why do you need to remember this fact and disable unswitching later?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3119-3120
+  for (auto &It : CandidatesULT)
+    Found |= insertCandidatesWithPendingInjections(
+        UnswitchCandidates, L, ICmpInst::ICMP_ULT, It.second, DT);
+  return Found;
----------------
Can this be done from the loop above?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3325-3332
   // If we didn't find any candidates, we're done.
-  if (!collectUnswitchCandidates(UnswitchCandidates, PartialIVInfo,
-                                 PartialIVCondBranch, L, LI, AA, MSSAU))
+  collectUnswitchCandidates(UnswitchCandidates, PartialIVInfo,
+                            PartialIVCondBranch, L, LI, AA, MSSAU);
+  collectUnswitchCandidatesWithInjections(UnswitchCandidates, PartialIVInfo,
+                                          PartialIVCondBranch, L, DT, LI, AA,
+                                          MSSAU);
+  if (UnswitchCandidates.empty())
----------------



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

https://reviews.llvm.org/D136233