[PATCH] D34583: [LSR] Narrow search space by filtering non-optimal formulae with the same ScaledReg and Scale.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:146
+static cl::opt<bool> FilterSameScaledReg(
+    "lsr-filter-same-scaledreg", cl::Hidden, cl::init(true),
+    cl::desc("Narrow LSR search space by filtering non-optimal formulae"
----------------
I'd s/scaledreg/scaled-reg/


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4318
+/// If a LSRUse has multiple formulae with the same ScaledReg and Scale.
+/// Pick the best one and delete the others.
+void LSRInstance::NarrowSearchSpaceByFilterFormulaWithSameScaledReg() {
----------------
Just to be clear, this benefit here is that you can just look at the base regs and (cheaply) decide which one is better?  If so, add a line in the description about that.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4338
+
+    auto isBetterThan = [&](Formula &FA, Formula &FB) {
+      // First we will try to choose the Formula with less new registers.
----------------
I think the convention would be `IsBetterThan`.

I'd also add a one-liner on the direction of the relation you're computing ("return true if A is better than B").


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4339
+    auto isBetterThan = [&](Formula &FA, Formula &FB) {
+      // First we will try to choose the Formula with less new registers.
+      float FARegNum = 0;
----------------
I'm being hyper pedantic here, but I suspect this will be "fewer" new registers.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4345
+      }
+      float FBRegNum = 0;
+      for (const SCEV *Reg : FB.BaseRegs) {
----------------
It's not clear to me why you need to use a floating point reciprocal here.  Why not just count the total number of uses (as a integer) and compare that?  Or is there some more subtle metric you're tracking that requires this reciprocal logic (if so please comment on that).


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4355
+      // less Cost.
+      DenseSet<const SCEV *> VisitedRegs;
+      SmallPtrSet<const SCEV *, 16> Regs;
----------------
Maybe the `DenseSet` can be declared at function scope and re-used in different invocations to `isBetterThan`?  This is a worthwhile micro-optimization since `DenseSet` s are somewhat heavyweight.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4360
+      Regs.clear();
+      CostFB.RateFormula(TTI, FB, Regs, VisitedRegs, L, SE, DT, LU);
+      return CostFA.isLess(CostFB, TTI);
----------------
As far as I can tell, in this situation we're making an arbitrary choice when the register use count is the same and both the formulas have the same cost (we'll just pick the second one).  Can we instead keep both the formulas when `Cost` does not give us a unambiguous signal?


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4364
+
+    bool Any = false;
+    for (size_t FIdx = 0, NumForms = LU.Formulae.size(); FIdx != NumForms;
----------------
Let's call `Any` something more descriptive, like `FormulaPruned`.

[edit: I just saw that other parts of LSR also call this variable `Any`.  I think those too should change, but given the "prior art" I'd be okay if you want to keep this named `Any`.]


Repository:
  rL LLVM

https://reviews.llvm.org/D34583