[PATCH] D120232: [SelectOpti][4/5] Loop Heuristics

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:458
+  //    reducing the loop's critical path by some threshold; and
+  // ii) the cost of the select group is cheaper (in terms of latency) compared
+  //     to the select version.
----------------
Not clear about what the second condition actually means. What is the cost of select group?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:468
+                                     L->getHeader()->getFirstNonPHI());
+    ORmissL << "Did not convert any select in the loop to branch due to "
+               "loop-level heuristics.";
----------------
Should this be missed optimization or Analysis. The message probably also needs more context to be useful.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:663
+  if (Diff[1] < Scaled64::get(GainCycleThreshold) &&
+      Diff[1] * Scaled64::get(8) < LoopCost[1].PredCost)
+    return false;
----------------
Add a parameter?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:668
+  // of the gain needs to be at least LoopGradientGain% (defaults to 25%).
+  if (Diff[1] > Diff[0] && (Diff[1] - Diff[0]) * Scaled64::get(100) <
+                               (LoopCost[1].PredCost - LoopCost[0].PredCost) *
----------------
Add explanation of this heuristic.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:678
+
+bool SelectOptimize::computeLoopCosts(
+    const Loop *L, const SelectGroups &SIGroups,
----------------
Document the return value.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:681
+    DenseMap<const Instruction *, CostInfo> &InstCostMap,
+    CostInfo LoopCost[2]) {
+  const auto &SIset = getSIset(SIGroups);
----------------
Perhaps passing by pointer explicitly?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:685
+  // both predicated and non-predicated version.
+  const unsigned Iterations = 2;
+  for (unsigned Iter = 0; Iter < Iterations; ++Iter) {
----------------
why only 2 iterations?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:713
+        IPredCost += Scaled64::get(ILatency.getValue());
+        INonPredCost += Scaled64::get(ILatency.getValue());
+
----------------
Should this exclude the selection instruction itself for non-pred case?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:741
+
+          INonPredCost = PredictedPathCost + MispredictCost;
+        }
----------------
should it weighted with misprediction rate?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:745
+        InstCostMap[&I] = {IPredCost, INonPredCost};
+        MaxCost.PredCost = std::max(MaxCost.PredCost, IPredCost);
+        MaxCost.NonPredCost = std::max(MaxCost.NonPredCost, INonPredCost);
----------------
Should Sum (of cost in a SI group) be used instead of max?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:787
+  Scaled64 MispredictCost =
+      std::max(Scaled64::get(MispredictPenalty), CondCost) *
+      Scaled64::get(MispredictRate);
----------------
Should it be scaled by CondCost instead of taking the max?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:807
+      PredPathCost /= Scaled64::get(SumWeight);
+    }
+  }
----------------
Should there be an early return here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120232