[PATCH] D73501: [SCEV] SCEVExpander::isHighCostExpansion(): perform actual cost-modelling (PR44668)

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

The general structure of the patch looks entirely reasonable.  I am concerned about the number of test diffs.  Mostly because there might be regressions herein, and it would be really hard to spot.  I'd like to suggest a strategy for making the test changes easier.

Split the patch into a couple of stages:

1. Add all the TTI plumbing, and one simple use of the costing (e.g. trunc), but keep most of isHighCostExpansion as is.  Intention is that very few tests change, and results are obviously correct.
2. Add remaining use of TTI in isHighCost except for the min/max (since those were unconditional failures previously).  Again, hopefully smallish number of diffs to review.
3. Add the min/max logic.  These are the most interesting diffs to study.
4. Change the bailout logic in IndVarSimplify.

If you're okay with this approach, consider yourself to have a conditional LGTM for (1) provided that the test diffs are actually very limited.

If you want to make an alternate suggestion as to how to review the test diffs effectively, I'm open to it.



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2156
+  if (auto *CastExpr = dyn_cast<SCEVCastExpr>(S)) {
+    unsigned Opcode;
+    switch (S->getSCEVType()) {
----------------
Minor: We may already have a SCEVType to OpCode conversion helper.  If we don't, we should probably create one and factor this out.  I'm sure we have the same logic elsewhere as well.  


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2184
+                                      BudgetRemaining, TTI, Processed) ||
+            isHighCostExpansionHelper(UDivExpr->getRHS(), L, At,
+                                      BudgetRemaining, TTI, Processed))
----------------
The second check here is redundant as you've already checked that RHS is a constant.



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2261
+        return true;
+      PairCost += CostIncreasePerStep;
+    }
----------------
Style wise, I'd suggest splitting AddRec into it's own case since it requires complexity the others don't.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1360
+            !isa<SCEVUnknown>(ExitValue) && hasHardUserWithinLoop(L, Inst) &&
+            HighCost)
           continue;
----------------
Can I ask you to split this piece into it's own separate patch?  The rest of NFC-ish, this really isn't.  

Also, you can remove the not-constant and not-unknown cases as that's now handled by the high cost clause.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:48
 
+static cl::opt<unsigned> ReplaceIWUserWithLoopInvariantBudget(
+    "replace-iv-user-with-loop-invariant-budget", cl::Hidden, cl::init(4),
----------------
Given we have the same magic number scatter around, I'd suggest moving this to a utility file and sharing the same constant param for all uses.  We can split later if useful.


================
Comment at: llvm/test/Transforms/IndVarSimplify/dont-recompute.ll:38
 ; CHECK:       for.end:
-; CHECK-NEXT:    [[ADD_LCSSA:%.*]] = phi i32 [ [[ADD]], [[FOR_BODY]] ]
-; CHECK-NEXT:    tail call void @func(i32 [[ADD_LCSSA]])
+; CHECK-NEXT:    [[TMP0:%.*]] = mul i32 [[M]], 186
+; CHECK-NEXT:    tail call void @func(i32 [[TMP0]])
----------------
This test change is against the intention of the file per the name.  I think when you split as suggested above, this will need consolidated, renamed, or something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73501