[RISCV] Return Invalid cost in getGatherScatterOpCost instead of crashing for scalable vectors

[liaolucy]

Summary

getCommonMaskedMemoryOpCost tries to cast<FixedVectorType>
on a scalable vector type. Return invalid instead.

[liaolucy]

I try to support scalable vectors in getGatherScatterOpCost, but I don't know how to support scalable vectors.

https://reviews.llvm.org/D115143 SVE adds on an overhead cost for gathers and scatters, which is a rough estimate based on performance investigations.

Does RVV add similar values？

[liaolucy]

https://reviews.llvm.org/D119529 maybe this patch , try to fix the same issue

[frasercrmck]

I actually hit this same issue the day you posted this, which is fun. But I fear this is quite a lot of work to get watertight. I've left some comments showing where we'd still crash. I've also seen us crash on getIntrinsicInstrCost when given, e.g., llvm.cttz.nxv1i8.

Looking at D119529, I'm not sure I see the value in accepting that the BaseTTI version is just allowed to crash on scalable vectors. It's a very common idiom that implementations fall back to that. It also just meaning extra work for new targets supporting scalable vectorization: now they need a fully-fledged TTI implementation to avoid crashing? Seems questionable to me.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

198

We can still crash here: I've seen this in CodeMetrics:analyzeBasicBlock because it calls with TCK_CodeSize.

201

We'd still crash here?

[liaolucy]

In D121677#3386240, @frasercrmck wrote:

I actually hit this same issue the day you posted this, which is fun. But I fear this is quite a lot of work to get watertight. I've left some comments showing where we'd still crash. I've also seen us crash on getIntrinsicInstrCost when given, e.g., llvm.cttz.nxv1i8.

Can adding a cost model for llvm.cttz.nxv1i8 solve this problem? I would like to try.

[frasercrmck]

In D121677#3418361, @liaolucy wrote:

Can adding a cost model for llvm.cttz.nxv1i8 solve this problem? I would like to try.

In my experience, the minimum we need to handle is:

1. any scalable-vector intrinsics in getTypeBasedIntrinsicInstrCost which crashes in the base version. This would fix cttz.
2. scalable-vector fshl,fshr,experimental_stepvector,experimental_vector_insert and experimental_vector_extract in getIntrinsicInstrCost

In #2, these intrinsics all call BaseT::getIntrinsicInstrCost on scalable-vectors, so we need to catch them ahead of time. The other intrinsics either return some kind of cost or fall through to getTypeBasedIntrinsicInstrCost which we'll handle in #1.

This is what I mean about it looking like a lot of work. I've done the bare minimum in our downstream to fix crashes but the costs I used are wildly inaccurate: I just don't expect us to crash on IR containing intrinsics.

[liaolucy]

After a series of @reames's patches, no crash, thanks.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

RISCVTTIImpl::getMaskedMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
 
   return getMemoryOpCost(Opcode, Src, Alignment, AddressSpace, CostKind);
 }
 
 InstructionCost RISCVTTIImpl::getGatherScatterOpCost(
     unsigned Opcode, Type *DataTy, const Value *Ptr, bool VariableMask,
     Align Alignment, TTI::TargetCostKind CostKind, const Instruction *I) {
   if (CostKind != TTI::TCK_RecipThroughput)
     return BaseT::getGatherScatterOpCost(Opcode, DataTy, Ptr, VariableMask,
                                          Alignment, CostKind, I);
 
   if ((Opcode == Instruction::Load &&
        !isLegalMaskedGather(DataTy, Align(Alignment))) ||
       (Opcode == Instruction::Store &&
        !isLegalMaskedScatter(DataTy, Align(Alignment))))
     return BaseT::getGatherScatterOpCost(Opcode, DataTy, Ptr, VariableMask,
                                          Alignment, CostKind, I);
 
   // FIXME: Only supporting fixed vectors for now.
   if (!isa<FixedVectorType>(DataTy))
-    return BaseT::getGatherScatterOpCost(Opcode, DataTy, Ptr, VariableMask,
-                                         Alignment, CostKind, I);
+    return InstructionCost::getInvalid();
 
   auto *VTy = cast<FixedVectorType>(DataTy);
   unsigned NumLoads = VTy->getNumElements();
   InstructionCost MemOpCost =
       getMemoryOpCost(Opcode, VTy->getElementType(), Alignment, 0, CostKind, I);
   return NumLoads * MemOpCost;
 }
 

llvm/test/Transforms/LoopVectorize/RISCV/masked_gather_scatter.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -loop-vectorize -mtriple=riscv32 -mattr=+v,+d -riscv-v-vector-bits-min=256 -S | FileCheck %s -check-prefixes=RV32
 ; RUN: opt < %s -loop-vectorize -mtriple=riscv64 -mattr=+v,+d -riscv-v-vector-bits-min=256 -S | FileCheck %s -check-prefixes=RV64
 
+; Check that we don't crash when scalable-vectorization=on.
+; RUN: opt < %s -loop-vectorize -mtriple=riscv32 -mattr=+v,+d -scalable-vectorization=on -S
+; RUN: opt < %s -loop-vectorize -mtriple=riscv64 -mattr=+v,+d -scalable-vectorization=on -S
+
 ; The source code:
 ;
 ;void foo4(double *A, double *B, int *trigger) {
 ;
 ;  for (int i=0; i<10000; i += 16) {
 ;    if (trigger[i] < 100) {
 ;          A[i] = B[i*2] + trigger[i]; << non-consecutive access
 ;    }