[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));
+
----------------
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(), ..)`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1164
+
+  VectorType *VTy = dyn_cast<VectorType>(Src);
+  if (VTy && !isLegalToVectorizeElementType(
----------------
david-arm wrote:
> nit: I think because we bail out at the start of the function if it's not a scalable vector we can actually just do:
> 
>   VectorType *VTy = cast<ScalableVectorType>(Src);
>   if (!isLegalToVectorizeElementType(...))
> 
> This is also true for getGatherScatterOpCost
nit: I'd prefer for this to be written as:

  if (!LT.first.isValid())
    return InstructionCost::getInvalid();
  return LT.first * 2;

so it's a little more explicit that Invalid is returned.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-illegal-types.ll:3
+
+define <vscale x 2 x i128> @load_nxvi128(<vscale x 2 x i128>* %val) {
+; CHECK-LABEL: 'load_nxvi128'
----------------
Can you structure these tests a bit more so that we check the code in TargetLoweringBase for each of the types:
  nxv1i128
  nxv2i128
  nxv1f128
  nxv2f128
using e.g. `load`, and then testing all the other load/store operations (store, masked.load, masked.store) with only nxv1i128.

You can also merge all the instructions into the same function, because for invoking the cost-model, it doesn't actually matter how the result values are used.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-hint.ll:5
+; RUN: opt -mtriple=aarch64-none-linux-gnu -loop-vectorize -pass-remarks-analysis=loop-vectorize -debug-only=loop-vectorize -S -scalable-vectorization=on < %s 2>%t | FileCheck --check-prefix=CHECK-NO-SVE %s
+; RUN: cat %t | FileCheck %s -check-prefix=CHECK-NO-SVE-REMARKS
 
----------------
Is it necessary to pipe the output to a temporary file and use a different check-prefix?


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

https://reviews.llvm.org/D102515