[PATCH] D91174: [Analysis] Introduce a new InstructionCost class

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:61
+
+  // For all of the arithmetic operators provided here any non-normal state is
+  // perpetuated and cannot be removed. Once a cost becomes invalid it stays
----------------



================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:80-84
+  // Things get a little strange here if we subtract an invalid value, since
+  // for the purpose of comparisons we treat 'invalid' as a positive infinitely
+  // expensive cost. The user has two options here:
+  //   1. Check for invalid values before subtracting, or
+  //   2. Always check the result is valid before doing comparisons.
----------------
I feel like this comment is unnecessarily scary. I believe that the intuition that you have formed is:

```
CostType evaluateCostState(CostState c) {
   if (c == Valid)
      return 0;
   return std::numeric_limits<CostType>::max()
}

CostType evaluateCost(InstructionCost c) {
   return c.Value + evaluateCostState(c.State);
}
```
 As you've noted, this falls apart for subtraction.

I think the intuition should be more like how floating point numbers work wrt NaN. For `l + r`, where l and r are floats, the user just assumes they do math like normal. NaN is just a cancer value that can contaminate a float if you do something bad. The user typically does not expect to be able to recover from NaN, they just make sure they don't have it at some point. Similarly, we expect users of InstructionCost to just do math with costs as if they were a regular scalar value. If they ask for the cost of something that should never be done, they get an invalid cost. For all arithmetic operators `op`, `l op r` is invalid for all `l` and `r`. At some point, the user will inspect a cost and see if it's valid, and make a choice based on this info.

The only different here is that unlike floats with NaNs, we have a useful total ordering. In this case, it's a lexicographical ordering, where `Invalid > Valid` and `State` is checked before `Value`.


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:157
+  bool operator!=(const CostType RHS) const { return !(*this == RHS); }
+
+  bool operator<(const InstructionCost &RHS) const {
----------------
Maybe it would be good to document the ordering and motivation for it?


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:160
+    if (isInvalid() || RHS.isInvalid())
+      return isInvalid() < RHS.isInvalid();
+    return Value < RHS.Value;
----------------
is `false < true` actually defined by the C++ standard? (I don't know offhand)


================
Comment at: llvm/include/llvm/Analysis/InstructionCost.h:42
+  T Value;
+  CostState State;
+
----------------
david-arm wrote:
> nikic wrote:
> > Please make "invalid" a special value (like -1) instead.
> Hi @nikic , do you mean remove the CostState and use a set of special values to define any possible state such as invalid? The reason I chose not to do this was for two reasons:
> 
> 1. If we ever want to support an additional state in future then it means another special value, e.g. -2. I thought it was better to avoid having a list of special values and simply encode the state separately.
> 2. There are plenty of examples in the codebase where costs can go negative, for example when calculating the cost budget remaining for some loop optimisations or in the SLPVectorizer. In such cases you could hit a value of -1 accidentally during normal operation and it would then be treated as invalid.
> 
> Does anyone else have any thoughts about this?
I think this approach is error-prone. Additionally, it's not extensible. Maybe we want to add scalable costs in the future? The way you have it defined now, it's a straightforward extension. If you have "invalid" just be a special case cost, then we have to repeat this whole exercise.

I feel strongly that your approach is preferable.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:257-258
     }
-    llvm_unreachable("Unknown instruction cost kind");
+    if (Cost == -1)
+      Cost.setInvalid();
+    return Cost;
----------------
Why is this being done? Did the code not handle -1 correctly before?


================
Comment at: llvm/unittests/Analysis/InstructionCostTest.cpp:23-28
+  InstructionCost Cost1 = 3;
+  InstructionCost Cost2 = -2;
+  InstructionCost Cost3 = 6;
+  InstructionCost Cost4 = InstructionCost::getInvalid(3);
+  InstructionCost Cost5 = InstructionCost::getInvalid(3);
+  InstructionCost TmpCost;
----------------
I would prefer that these costs have descriptive names. It's much more obvious why `EXPECT_GT(VThree, VNegTwo)` is true than `EXPECT_GT(Cost1, Cost2)`


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

https://reviews.llvm.org/D91174