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

[Sander de Smalen via Phabricator via llvm-commits]

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


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

https://reviews.llvm.org/D102515