[RISCV] Consolidate legality checking for strided load/store [nfc]

[reames]

Summary

Note that the strided load from concat_vector combine was using the wrong legality test. It happened to work out as the alignment requirement is based on the scalar type either way, but unless I'm missing something allowsMisalignedAccess is expecting a contiguous memory access.

Posting for review mostly to see if anyone had a better suggestion than getting the LLVM type from the VT. That's a bit ugly, but I didn't see a cleaner way as the element type check is in terms of Type, not MVT.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

Is WideScalarBitWidth guaranteed to be a valid MVT? Like can we get i256, i512, i1024, etc. here?

llvm/lib/Target/RISCV/RISCVISelLowering.h

709

Exceeds 80 columns I think

[luke]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

I presume if it's not then WideVecVT won't be legal below and we'll fail the legality check

[luke]

LGTM on my end

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

15811

Maybe we could refactor useRVVForFixedLengthVectors to use MVTs and have the Type users call getValueType. But that's for another patch.

[reames]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

This was my reasoning as well.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

My concern is that MVT::getIntegerVT might assert first.

[reames]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

Just to be clear, you're talking about a possible bug in the code before my change right? I don't see anything in this diff which sounds relevant to your last clarification.

If so, yes, I agree that looks suspicious.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

Yes it would be a bug before your change. Your patch just made me focus more on the type checking than I did in the original review.

[luke]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

11420

Argh, I forgot these were MVTs not EVTs.

llvm/lib/Target/RISCV/RISCVGatherScatterLowering.cpp

void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<LoopInfoWrapperPass>();
   }
 
   StringRef getPassName() const override {
     return "RISC-V gather/scatter lowering";
   }
 
 private:
-  bool isLegalTypeAndAlignment(Type *DataType, Value *AlignOp);
-
   bool tryCreateStridedLoadStore(IntrinsicInst *II, Type *DataType, Value *Ptr,
                                  Value *AlignOp);
 
   std::pair<Value *, Value *> determineBaseAndStride(GetElementPtrInst *GEP,
                                                      IRBuilder<> &Builder);
 
   bool matchStridedRecurrence(Value *Index, Loop *L, Value *&Stride,
                               PHINode *&BasePtr, BinaryOperator *&Inc,
                               IRBuilder<> &Builder);
 };
 
 } // end anonymous namespace
 
 char RISCVGatherScatterLowering::ID = 0;
 
 INITIALIZE_PASS(RISCVGatherScatterLowering, DEBUG_TYPE,
                 "RISC-V gather/scatter lowering pass", false, false)
 
 FunctionPass *llvm::createRISCVGatherScatterLoweringPass() {
   return new RISCVGatherScatterLowering();
 }
 
-bool RISCVGatherScatterLowering::isLegalTypeAndAlignment(Type *DataType,
-                                                         Value *AlignOp) {
-  Type *ScalarType = DataType->getScalarType();
-  if (!TLI->isLegalElementTypeForRVV(ScalarType))
-    return false;
-
-  MaybeAlign MA = cast<ConstantInt>(AlignOp)->getMaybeAlignValue();
-  if (MA && MA->value() < DL->getTypeStoreSize(ScalarType).getFixedValue())
-    return false;
-
-  // FIXME: Let the backend type legalize by splitting/widening?
-  EVT DataVT = TLI->getValueType(*DL, DataType);
-  if (!TLI->isTypeLegal(DataVT))
-    return false;
-
-  return true;
-}
-
 // TODO: Should we consider the mask when looking for a stride?
 static std::pair<Value *, Value *> matchStridedConstant(Constant *StartC) {
   if (!isa<FixedVectorType>(StartC->getType()))
     return std::make_pair(nullptr, nullptr);
 
   unsigned NumElts = cast<FixedVectorType>(StartC->getType())->getNumElements();
 
   // Check that the start value is a strided constant.
RISCVGatherScatterLowering::determineBaseAndStride(GetElementPtrInst *GEP,
   return P;
 }
 
 bool RISCVGatherScatterLowering::tryCreateStridedLoadStore(IntrinsicInst *II,
                                                            Type *DataType,
                                                            Value *Ptr,
                                                            Value *AlignOp) {
   // Make sure the operation will be supported by the backend.
-  if (!isLegalTypeAndAlignment(DataType, AlignOp))
+  MaybeAlign MA = cast<ConstantInt>(AlignOp)->getMaybeAlignValue();
+  if (!MA || !TLI->isLegalStridedLoadStore(*DL, DataType, *MA))
+    return false;
+
+  // FIXME: Let the backend type legalize by splitting/widening?
+  if (!TLI->isTypeLegal(TLI->getValueType(*DL, DataType)))
     return false;
 
   // Pointer should be a GEP.
   auto *GEP = dyn_cast<GetElementPtrInst>(Ptr);
   if (!GEP)
     return false;
 
   IRBuilder<> Builder(GEP);

llvm/lib/Target/RISCV/RISCVISelLowering.h

public:
   /// returns the address of that location. Otherwise, returns nullptr.
   Value *getIRStackGuard(IRBuilderBase &IRB) const override;
 
   /// Returns whether or not generating a fixed length interleaved load/store
   /// intrinsic for this type will be legal.
   bool isLegalInterleavedAccessType(FixedVectorType *, unsigned Factor,
                                     const DataLayout &) const;
 
+  /// Return true if a stride load store of the given result type and
+  /// alignment is legal.
+  bool isLegalStridedLoadStore(const DataLayout &DL, Type *DataType, Align Alignment) const;
+
   unsigned getMaxSupportedInterleaveFactor() const override { return 8; }
 
   bool lowerInterleavedLoad(LoadInst *LI,
                             ArrayRef<ShuffleVectorInst *> Shuffles,
                             ArrayRef<unsigned> Indices,
                             unsigned Factor) const override;
 
   bool lowerInterleavedStore(StoreInst *SI, ShuffleVectorInst *SVI,

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

if (auto *ConstStride = dyn_cast<ConstantSDNode>(Stride);
 
     SDValue WideLoad = DAG.getLoad(VT, DL, BaseLd->getChain(), BasePtr, MMO);
     for (SDValue Ld : N->ops())
       DAG.makeEquivalentMemoryOrdering(cast<LoadSDNode>(Ld), WideLoad);
     return WideLoad;
   }
 
   // Get the widened scalar type, e.g. v4i8 -> i64
   unsigned WideScalarBitWidth =
       BaseLdVT.getScalarSizeInBits() * BaseLdVT.getVectorNumElements();
   MVT WideScalarVT = MVT::getIntegerVT(WideScalarBitWidth);
 
   // Get the vector type for the strided load, e.g. 4 x v4i8 -> v4i64
   MVT WideVecVT = MVT::getVectorVT(WideScalarVT, N->getNumOperands());
   if (!TLI.isTypeLegal(WideVecVT))
     return SDValue();
 
+  // Check that the operation is legal
+  Type *WideVecTy = EVT(WideVecVT).getTypeForEVT(*DAG.getContext());
+  if (!TLI.isLegalStridedLoadStore(DAG.getDataLayout(), WideVecTy, Align))
+    return SDValue();
+
   MVT ContainerVT = TLI.getContainerForFixedLengthVector(WideVecVT);
   SDValue VL =
       getDefaultVLOps(WideVecVT, ContainerVT, DL, DAG, Subtarget).second;
   SDVTList VTs = DAG.getVTList({ContainerVT, MVT::Other});
   SDValue IntID =
       DAG.getTargetConstant(Intrinsic::riscv_vlse, DL, Subtarget.getXLenVT());
   SDValue Ops[] = {BaseLd->getChain(),
                    IntID,
static SDValue performCONCAT_VECTORSCombine(SDNode *N, SelectionDAG &DAG,
   else
     // If Stride isn't constant, then we can't know how much it will load
     MemSize = MemoryLocation::UnknownSize;
 
   MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
       BaseLd->getPointerInfo(), BaseLd->getMemOperand()->getFlags(), MemSize,
       Align);
 
-  // Can't do the combine if the common alignment isn't naturally aligned with
-  // the new element type
-  if (!TLI.allowsMemoryAccessForAlignment(*DAG.getContext(),
-                                          DAG.getDataLayout(), WideVecVT, *MMO))
-    return SDValue();
-
   SDValue StridedLoad = DAG.getMemIntrinsicNode(ISD::INTRINSIC_W_CHAIN, DL, VTs,
                                                 Ops, WideVecVT, MMO);
   for (SDValue Ld : N->ops())
     DAG.makeEquivalentMemoryOrdering(cast<LoadSDNode>(Ld), StridedLoad);
 
   // Note: Perform the bitcast before the convertFromScalableVector so we have
   // balanced pairs of convertFromScalable/convertToScalable
   SDValue Res = DAG.getBitcast(
bool RISCVTargetLowering::isLegalInterleavedAccessType(
   // Need to make sure that EMUL * NFIELDS ≤ 8
   MVT ContainerVT = getContainerForFixedLengthVector(VT.getSimpleVT());
   auto [LMUL, Fractional] = RISCVVType::decodeVLMUL(getLMUL(ContainerVT));
   if (Fractional)
     return true;
   return Factor * LMUL <= 8;
 }
 
+bool RISCVTargetLowering::isLegalStridedLoadStore(const DataLayout &DL,
+                                                  Type *DataType,
+                                                  Align Alignment) const {
+  if (!Subtarget.hasVInstructions())
+    return false;
+
+  // Only support fixed vectors if we know the minimum vector size.
+  if (isa<FixedVectorType>(DataType) && !Subtarget.useRVVForFixedLengthVectors())
+    return false;
+
+  Type *ScalarType = DataType->getScalarType();
+  if (!isLegalElementTypeForRVV(ScalarType))
+    return false;
+
+  if (Alignment < DL.getTypeStoreSize(ScalarType).getFixedValue())
+    return false;
+
+  return true;
+}
+
 /// Lower an interleaved load into a vlsegN intrinsic.
 ///
 /// E.g. Lower an interleaved load (Factor = 2):
 /// %wide.vec = load <8 x i32>, <8 x i32>* %ptr
 /// %v0 = shuffle %wide.vec, undef, <0, 2, 4, 6>  ; Extract even elements
 /// %v1 = shuffle %wide.vec, undef, <1, 3, 5, 7>  ; Extract odd elements
 ///
 /// Into: