[SLP] Explicitly pass AccessTy to getGEPCost

[luke]

Summary

Building on D149889, this patch updates SLP to pass the vector type as
the AccessTy to getGEPCost.
This should have the effect of GEPs being costed for more often instead
of being treated as foldable into the address mode and thus free, as
some architectures, notably RISC-V, do not have offset+reg addressing
modes for vector memory accesses.

Note that in SLP, GEPs are costed in two places: getPointersChainCost
and GetGEPCostDiff.

[luke]

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

1644

Note to self: Should probably get around to deduplicating the getPointersChainCost here and the one in targettransforminfoimpl.h at some point.

[reames]

Masking this as request changes to get off review queues until dependent patches have landed. Please "request review" once blocking patches have landed.

I took only a very brief look at this, but I suspect further splitting will be required.

[ABataev]

llvm/include/llvm/Analysis/TargetTransformInfo.h

27

Do you really need this include in the header file?

[reames]

Please split the change in lib/Transforms/Scalar to their own review. They aren't connected to SLP, need different reviewers, and don't appear to have any test coverage in this change. They are *not* NFC due to the addition of the accessty inference.

[luke]

llvm/include/llvm/Analysis/TargetTransformInfo.h

27

I believe I had to include to use GetElementPtrInst, but now that I think about it maybe it's possible to just use GEPOperator instead.

[luke]

Split out non-SLP changes

[reames]

At this point, I'm going defer the remaining review to Alexey. What you've got looks entirely reasonable to me, but Alexey knows the SLP interactions much better.

[ABataev]

Just passing required argument looks good to me.

[luke]

In D153570#4460419, @ABataev wrote:

Just passing required argument looks good to me.

Thanks for the review: Just wanted to double check if you've approved this? I got an email that saying that you accepted the revision, but it still seems to be marked as needs review in Phabricator

[ABataev]

The patch is accepted

llvm/include/llvm/Analysis/TargetTransformInfo.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_TARGETTRANSFORMINFO_H
 #define LLVM_ANALYSIS_TARGETTRANSFORMINFO_H
 
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/IR/FMF.h"
 #include "llvm/IR/InstrTypes.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/InstructionCost.h"
 #include <functional>
 #include <optional>
 #include <utility>
public:
   /// folded into the addressing mode of a load/store. If AccessType is null,
   /// then the resulting target type based off of PointeeType will be used as an
   /// approximation.
   InstructionCost
   getGEPCost(Type *PointeeType, const Value *Ptr,
              ArrayRef<const Value *> Operands, Type *AccessType = nullptr,
              TargetCostKind CostKind = TCK_SizeAndLatency) const;
 
+  // Helper function for using getGEPCost on an already materialized GEP
+  // instruction.
+  InstructionCost
+  getGEPCost(const GetElementPtrInst *GEP,
+             TargetCostKind CostKind = TCK_SizeAndLatency) const {
+    Type *AccessType = nullptr;
+    // For now, only provide the AccessType in the simple case where the GEP
+    // only has one user.
+    if (GEP->hasOneUser())
+      AccessType = GEP->user_back()->getAccessType();
+    return getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),
+                      SmallVector<const Value *>(GEP->indices()), AccessType,
+                      CostKind);
+  }
+
   /// Describe known properties for a set of pointers.
   struct PointersChainInfo {
     /// All the GEPs in a set have same base address.
     unsigned IsSameBaseAddress : 1;
     /// These properties only valid if SameBaseAddress is set.
     /// True if all pointers are separated by a unit stride.
     unsigned IsUnitStride : 1;
     /// True if distance between any two neigbouring pointers is a known value.

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

for (const Value *V : Ptrs) {
         Cost += static_cast<T *>(this)->getArithmeticInstrCost(
             Instruction::Add, GEP->getType(), CostKind,
             {TTI::OK_AnyValue, TTI::OP_None}, {TTI::OK_AnyValue, TTI::OP_None},
             std::nullopt);
       } else {
         SmallVector<const Value *> Indices(GEP->indices());
         Cost += static_cast<T *>(this)->getGEPCost(GEP->getSourceElementType(),
                                                    GEP->getPointerOperand(),
-                                                   Indices, nullptr, CostKind);
+                                                   Indices, AccessTy, CostKind);
       }
     }
     return Cost;
   }
 
   InstructionCost getInstructionCost(const User *U,
                                      ArrayRef<const Value *> Operands,
                                      TTI::TargetCostKind CostKind) {

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

if (Info.isSameBase() && V != Base) {
         continue;
       Cost += getArithmeticInstrCost(Instruction::Add, GEP->getType(), CostKind,
                                      {TTI::OK_AnyValue, TTI::OP_None},
                                      {TTI::OK_AnyValue, TTI::OP_None},
                                      std::nullopt);
     } else {
       SmallVector<const Value *> Indices(GEP->indices());
       Cost += getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),
-                         Indices, nullptr, CostKind);
+                         Indices, AccessTy, CostKind);
     }
   }
   return Cost;
 }
 
 void RISCVTTIImpl::getUnrollingPreferences(Loop *L, ScalarEvolution &SE,
                                            TTI::UnrollingPreferences &UP,
                                            OptimizationRemarkEmitter *ORE) {

llvm/lib/Transforms/Scalar/NaryReassociate.cpp

if ((ResI = matchAndReassociateMinOrMax<umin_pred_ty>(I, OrigSCEV)) ||
         (ResI = matchAndReassociateMinOrMax<smax_pred_ty>(I, OrigSCEV)))
       return ResI;
 
   return nullptr;
 }
 
 static bool isGEPFoldable(GetElementPtrInst *GEP,
                           const TargetTransformInfo *TTI) {
-  SmallVector<const Value *, 4> Indices(GEP->indices());
-  return TTI->getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),
-                         Indices) == TargetTransformInfo::TCC_Free;
+  return TTI->getGEPCost(GEP) == TargetTransformInfo::TCC_Free;
 }
 
 Instruction *NaryReassociatePass::tryReassociateGEP(GetElementPtrInst *GEP) {
   // Not worth reassociating GEP if it is foldable.
   if (isGEPFoldable(GEP, TTI))
     return nullptr;
 
   gep_type_iterator GTI = gep_type_begin(*GEP);

llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp

return (Basis.Ins != C.Ins && // skip the same instruction
           DT->dominates(Basis.Ins->getParent(), C.Ins->getParent()) &&
           // They share the same base, stride, and candidate kind.
           Basis.Base == C.Base && Basis.Stride == C.Stride &&
           Basis.CandidateKind == C.CandidateKind);
 }
 
 static bool isGEPFoldable(GetElementPtrInst *GEP,
                           const TargetTransformInfo *TTI) {
-  SmallVector<const Value *, 4> Indices(GEP->indices());
-  return TTI->getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),
-                         Indices) == TargetTransformInfo::TCC_Free;
+  return TTI->getGEPCost(GEP) == TargetTransformInfo::TCC_Free;
 }
 
 // Returns whether (Base + Index * Stride) can be folded to an addressing mode.
 static bool isAddFoldable(const SCEV *Base, ConstantInt *Index, Value *Stride,
                           TargetTransformInfo *TTI) {
   // Index->getSExtValue() may crash if Index is wider than 64-bit.
   return Index->getBitWidth() <= 64 &&
          TTI->isLegalAddressingMode(Base->getType(), nullptr, 0, true,

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

if (isa<LoadInst, StoreInst>(VL0)) {
                    auto *Ptr = dyn_cast<GetElementPtrInst>(V);
                    return Ptr && !Ptr->hasAllConstantIndices();
                  })
               ? TTI::PointersChainInfo::getUnknownStride()
               : TTI::PointersChainInfo::getKnownStride();
 
       ScalarCost = TTI->getPointersChainCost(Ptrs, BasePtr, PtrsInfo, ScalarTy,
                                              CostKind);
-
-      // Remark: it not quite correct to use scalar GEP cost for a vector GEP,
-      // but it's not clear how to do that without having vector GEP arguments
-      // ready.
-      // Perhaps using just TTI::TCC_Free/TTI::TCC_Basic would be better option.
-      if (const auto *Base = dyn_cast<GetElementPtrInst>(BasePtr)) {
-        SmallVector<const Value *> Indices(Base->indices());
-        VecCost = TTI->getGEPCost(Base->getSourceElementType(),
-                                  Base->getPointerOperand(), Indices, nullptr,
+      if (auto *BaseGEP = dyn_cast<GEPOperator>(BasePtr)) {
+        SmallVector<const Value *> Indices(BaseGEP->indices());
+        VecCost = TTI->getGEPCost(BaseGEP->getSourceElementType(),
+                                  BaseGEP->getPointerOperand(), Indices, VecTy,
                                   CostKind);
       }
     }
 
     LLVM_DEBUG(dumpTreeCosts(E, 0, VecCost, ScalarCost,
                              "Calculated GEPs cost for Tree"));
 
     return VecCost - ScalarCost;

llvm/test/Transforms/SLPVectorizer/RISCV/gep.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=slp-vectorizer -mtriple=riscv64 -mattr=+v \
 ; RUN: -riscv-v-slp-max-vf=0 -S | FileCheck %s
 
-; FIXME: This should not be vectorized, as the cost of computing the
-; offsets nullifies the benefits of vectorizing:
+; This should not be vectorized, as the cost of computing the offsets nullifies
+; the benefits of vectorizing:
 ;
 ; copy_with_offset_v2i8:
 ;         addi    a0, a0, 8
 ;         vsetivli        zero, 2, e8, mf8, ta, ma
 ;         vle8.v  v8, (a0)
 ;         addi    a1, a1, 16
 ;         vse8.v  v8, (a1)
 ;         ret
 ;
 ; Compared to the scalar version where the offsets can be folded into the
 ; addressing mode:
 ;
 ; copy_with_offset_v2i8:
 ;         lbu     a2, 8(a0)
 ;         lbu     a0, 9(a0)
 ;         sb      a2, 16(a1)
 ;         sb      a0, 17(a1)
 ;   ret
 
 define void @copy_with_offset_v2i8(ptr noalias %p, ptr noalias %q) {
 ; CHECK-LABEL: @copy_with_offset_v2i8(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[P1:%.*]] = getelementptr i8, ptr [[P:%.*]], i32 8
+; CHECK-NEXT:    [[X1:%.*]] = load i8, ptr [[P1]], align 1
 ; CHECK-NEXT:    [[Q1:%.*]] = getelementptr i8, ptr [[Q:%.*]], i32 16
-; CHECK-NEXT:    [[TMP0:%.*]] = load <2 x i8>, ptr [[P1]], align 1
-; CHECK-NEXT:    store <2 x i8> [[TMP0]], ptr [[Q1]], align 1
+; CHECK-NEXT:    store i8 [[X1]], ptr [[Q1]], align 1
+; CHECK-NEXT:    [[P2:%.*]] = getelementptr i8, ptr [[P]], i32 9
+; CHECK-NEXT:    [[X2:%.*]] = load i8, ptr [[P2]], align 1
+; CHECK-NEXT:    [[Q2:%.*]] = getelementptr i8, ptr [[Q]], i32 17
+; CHECK-NEXT:    store i8 [[X2]], ptr [[Q2]], align 1
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %p1 = getelementptr i8, ptr %p, i32 8
   %x1 = load i8, ptr %p1
   %q1 = getelementptr i8, ptr %q, i32 16
   store i8 %x1, ptr %q1