[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 ↗

(On Diff #533653)

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 ↗

(On Diff #533653)

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/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/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