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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Thank you for taking a look!

In D73501#1844914 <https://reviews.llvm.org/D73501#1844914>, @reames wrote:

> The general structure of the patch looks entirely reasonable.


Aha. That was the main question here.

> 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 agree that the test changes do look unreviewable.

The main question may be: what do we consider a regression?
"Previously we considered this SCEV high-cost and now we don't" won't work well,
because of the nature of the patch - we go from semi-arbitrary rules
to "consistent" application of "consistent" cost model.
Naturally, some patterns will no longer be high-cost, and some will become high-cost.

For example, i believe all of the min/max+loop latch changes fall into this category,
and i suspect that the majority of changes are of that nature.

"Cost-modelling is incorrect, SCEV expression should have cost N but we counted it as K"
This is the main problem IMO. This might need unittests for `isHighCostExpansion()` cost-modelling.

> 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.

I'll explore ways to make diffs more reviewable.



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2156
+  if (auto *CastExpr = dyn_cast<SCEVCastExpr>(S)) {
+    unsigned Opcode;
+    switch (S->getSCEVType()) {
----------------
reames wrote:
> 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.  
We don't necessarily have 1:1 mapping between SCEV type and an IR instruction.
(E.g. `scAddRecExpr`, and even min/max)
What should that helper be doing in that case?


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:2261
+        return true;
+      PairCost += CostIncreasePerStep;
+    }
----------------
reames wrote:
> Style wise, I'd suggest splitting AddRec into it's own case since it requires complexity the others don't.  
Also, this overcharges `scAddRecExpr` if `Op` is actually a zero constant.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73501