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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Mostly looks good, Some comments with nits, please consider.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2884
+/// injecting a loop-invariant condition.
+static bool shouldInjectInvariantCondition(const ICmpInst::Predicate Pred,
+                                           const Value *LHS, const Value *RHS,
----------------
shouldTryInjectInvariantCondition?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2905
+/// Returns true, if metadata on \p BI allows to optimize branching into \p
+/// TakenSucc via injection of invariant conditions.
+bool shouldInjectBasingOnMetadata(const BranchInst *BI,
----------------
May we just document the real behavior?
That we do not want to unswitch on branches with probability less than some very likely threshold?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2906
+/// TakenSucc via injection of invariant conditions.
+bool shouldInjectBasingOnMetadata(const BranchInst *BI,
+                                  const BasicBlock *TakenSucc) {
----------------
shouldTryInjectBasingOnMetadata


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2915
+    return false;
+  MDNode *ProfileData = getValidBranchWeightMDNode(*BI);
+  if (!ProfileData)
----------------
why not just use
if (!extractBranchWeights(const Instruction &I, SmallVectorImpl<uint32_t> &Weights)
  return false;


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2918
+    return false;
+  BranchProbability LikelyTaken(15, 16);
+
----------------
An Option instead of hardcoded probability?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3013
+
+#ifndef NDEBUG
+  DT.verify();
----------------
May be do this verification only for expensive debug?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3040
+///   ...
+/// Though they don't immediately exist in the IR, we can still inject them.
+static bool insertCandidatesWithPendingInjections(
----------------
What if they exist? do you expect that it is already simplified?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3324
   Instruction *PartialIVCondBranch = nullptr;
+  collectUnswitchCandidates(UnswitchCandidates, PartialIVInfo,
+                            PartialIVCondBranch, L, LI, AA, MSSAU);
----------------
collectUnswitchCandidates has only this use. you probably want to make it returning void now. you can land it as a separate NFC patch.


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

https://reviews.llvm.org/D136233