[PATCH] D102515: [CostModel] Return an invalid cost for memory ops with unsupported types

Mon Jun 7 04:39:07 PDT 2021

sdesmalen added inline comments.

================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1826-1828
+    if (LK.first == TypeScalarizeScalableVector)
+      return std::make_pair(InstructionCost::getInvalid(), MVT::getVT(Ty));
+
----------------
dfukalov wrote:
> sdesmalen wrote:
> > dfukalov wrote:
> > > sdesmalen wrote:
> > > > dfukalov wrote:
> > > > > sdesmalen wrote:
> > > > > > kmclaughlin wrote:
> > > > > > > dfukalov wrote:
> > > > > > > > My observation was that here less destructive for test is to just `Cost.setInvalid()` instead of return 'empty' invalid value. So the function continues to return the same numeric value but with `invalid` flag. It will create less impact on the current flow.
> > > > > > > > 
> > > > > > > > Most of operations with `InstructionCost` are not aware of invalid flag. I guess it might be be next separated step to stop loop here and just return invalid value and then gather all side effects of 'changed' cost numeric value.
> > > > > > > > 
> > > > > > > > Btw my D102915, D103407 and D103406 are preparation to return invalid cost flag from the function and to reduce impact of the change.
> > > > > > > Hi @dfukalov, thanks for the suggestion. I have updated this to instead set the cost to Invalid where the kind is TypeScalarizeScalableVector for now.
> > > > > > The value of Invalid is irrelevant when the Invalid flag is set. In fact, retrieving the value is not possible since `InstructionCost::getValue()` returns an `llvm::Optional`. Because there is nothing the code below can do to change the invalid cost to a valid cost, there's no reason not to break out of the loop early by returning `std::make_pair(InstructionCost::getInvalid(), ..)`.
> > > > > Actually almost all operations on `InstructionCost` ignore invalid flag at the moment. I don't insist, just suggested to split the changes to smaller steps to reduce future side effects of a commit.
> > > > Perhaps I misunderstand your concern, do you mean that if InstructionCost has a value like "10" but the value has been set to 'Invalid', that the original code which called `getTypeLegalizationCost ` will continue to operate on the value "10"?  That's not really how InstructionCost works, once the value is Invalid, it will never become Valid. i.e. Invalid * 2 is still Invalid. It's also not possible to retrieve the original value "10", because InstructionCost::getValue() returns an Optional, which if the cost is Invalid, will be None.
> > > > 
> > > > So even if most operations on InstructionCost are not aware of the Invalid flag, they mostly just continue propagating "invalid", and this will eventually bubble up to top-level call where it needs to do somethign with the value (e.g. by calling 'getValue()''). These instances should already be able to deal with Invalid, and if it does cause a crash, then this is something we'll need to fix.
> > > > 
> > > > At the moment though, the only case where this patch may have an impact are with scalable vectors in combination with illegal vectors. This combination is still very experimental, so if this does end up breaking anything it just highlights something that needs fixing.
> > > There are a lot of places where logic is based on comparison of `InstructionCost` with an integer or at least between two costs. In D103406 I experimented and renamed `getValue()` - it's used in a few dozens places.
> > > It seems all other code (like comparisons and arithmetic operations) still ignore invalid flag. It will be next, I guess painful, step to check invalid (and assert?) at least in comparison with integers.
> > > 
> > > As an illustration of the impact, you can check [[ https://reviews.llvm.org/harbormaster/unit/106498/ | test report of the previous patch version ]]. I don't know how many regressions will be in cases not covered with tests.
> > > 
> > > Again, I don't insist, but it seems to me if a cost model change with unpredicted regressions can be splitted to smaller patches - it would be better to split it.
> > > There are a lot of places where logic is based on comparison of InstructionCost with an integer or at least between two costs. In D103406 I experimented and renamed getValue() - it's used in a few dozens places.
> > > It seems all other code (like comparisons and arithmetic operations) still ignore invalid flag. It will be next, I guess painful, step to check invalid (and assert?) at least in comparison with integers.
> > InstructionCost has overloaded comparison- and arithmetic operators. The comparison operators check the Invalid flag. e.g. for some `auto X = InstructionCost::(42).setInvalid();`, then `(X == 42)  <=> false`. See the implementation here: https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/llvm/include/llvm/Support/InstructionCost.h#L169 .
> > 
> > The arithmetic operators propagate the Invalid flag, so that `X.isValid() == (X * 2).isValid()`.
> > 
> > > As an illustration of the impact, you can check test report of the previous patch version. I don't know how many regressions will be in cases not covered with tests.
> > @kmclaughlin explained to me off-list that these tests needed fixing in the previous revision of the patch, and so with the current patch the tests should still pass when changing the code back to return `InstructionCost::getInvalid()`.
> > 
> > Thanks for raising these concerns, it's good to check we're not missing anything. But I hope the above explains why it should be safe to return `getInvalid()` directly.
> > InstructionCost has overloaded comparison- and arithmetic operators. The comparison operators check the Invalid flag. e.g. for some auto X = InstructionCost::(42).setInvalid();, then (X == 42)  <=> false.
> 
> Yes. But at the same time for some `auto X = InstructionCost::(42).setInvalid();` we have funny `X > 142 <=> true`, since it finally calls `InstructionCost(142).operator<(X)` that has `if (State != RHS.State) return State < RHS.State;` at the start. I guess there are a lot of places with "cost > threshold" comparisons.
> It seems we need to fix InstructionCost comparisons (and other operations?) before the change or we'll get side effects. 
> 
> Actually I thought any operation with invalid cost should cause assert at some point in beautiful future. Am I right it was intended?
> Yes. But at the same time for some auto X = InstructionCost::(42).setInvalid(); we have funny X > 142 <=> true, since it finally calls InstructionCost(142).operator<(X) that has if (State != RHS.State) return State < RHS.State; at the start. I guess there are a lot of places with "cost > threshold" comparisons.
That's actually a feature, not a bug :) We basically had the option of always asserting that both costs need to be valid (which would lead to lots of extra code to always guarantee this by checking `X.isValid() && Y.isValid()` in lots of places before being able to compare the two costs. The other option was to have a documented total ordering where Invalid is considered 'infinitely expensive' when compared with any other costs. This is useful, because in most places the comparison is there to check if the cost of "some operation" is an improvement over the cost of some other operation. And an Invalid cost can never be an improvement to a valid cost.

> It seems we need to fix InstructionCost comparisons (and other operations?) before the change or we'll get side effects.
Most places have already been fixed in the passes that use InstructionCost. Perhaps there are some cases we've missed, but hopefully these show up easily enough in our testing. I expect most of these cases are actually gaps in our cost-model for scalable vectors, which is currently the only reason we ever return Invalid. For fixed-width vectors or anything else, we should always have valid costs.

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

https://reviews.llvm.org/D102515