[PATCH] D105108: [InstructionCost] Add saturation support.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D105108#2848042 <https://reviews.llvm.org/D105108#2848042>, @kparzysz wrote:

> If hitting either one of the saturation boundaries was realistic enough to warrant this change, then why having both, large positive and large negative costs wouldn't be?  If cost1 > 0 and cost2 < 0, and they both exceed the bounds, then cost1+cost2 in the saturating arithmetic may be a finite number, reasonably close to 0, and yet completely meaningless.
>
> Edit: clarified the saturated arithmetic

You're right that if values are both positive and negative that the resulting value is meaningless if these values are both exceeding their bounds. It's not the common use-case though, since most places just accumulate costs and expect all instruction costs to be positive values (the same holds for places that subtract (all positive) costs from some budget). I think the only exception is the SLPVectorizer, which does both addition/subtraction.

One the motivations for this patch was the overflow in the LoopVectorizer when applying D105113 <https://reviews.llvm.org/D105113>. This tries to implement the cost-comparison on InstructionCost, rather than doing it on InstructionCost::CostTy and then extending the values to int64_t before multiplication. The starting cost is set to `std::numeric_limits<InstructionCost::CostTy>::max()` which when multiplied by the vector element-count overflows. This could practically be resolved by setting that value to some lower value, but there may be other places where multiplication with e.g. MaxTripCounts could lead to overflow.

The common case for InstructionCost is asking "what is the cost of <operation> or <set of operations>?", which should always be a positive number. Conceptually it makes little sense to have negative costs to begin with, so at some point I'd like to see InstructionCost become unsigned to avoid such complications. Perhaps a feasible route would be to make InstructionCost unsigned and saturating by default and have a separate `SignedWrappingInstructionCost` class for any exceptions where signedness and wrapping may be required? If so, that would be a bigger piece of work than what this patch tried to achieve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105108