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

Wed Mar 16 08:21:21 PDT 2022

frasercrmck added a comment.
Herald added subscribers: s, arichardson.

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 <https://reviews.llvm.org/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.

================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:198
   if (CostKind != TTI::TCK_RecipThroughput)
     return BaseT::getGatherScatterOpCost(Opcode, DataTy, Ptr, VariableMask,
                                          Alignment, CostKind, I);
----------------
We can still crash here: I've seen this in `CodeMetrics:analyzeBasicBlock` because it calls with `TCK_CodeSize`.

================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:201

   if ((Opcode == Instruction::Load &&
        !isLegalMaskedGather(DataTy, Align(Alignment))) ||
----------------
We'd still crash here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121677