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


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

https://reviews.llvm.org/D102515