[SVE][AArch64] Fix TypeSize warning in GEP cost analysis

[joechrisellis]

Summary

The warning would fire when calling getGEPCost for analyzing the cost of
a GEP instruction. This would result in the use of the now deprecated
implicit cast of TypeSize to uint64_t through the overloaded operator.

This patch fixes the issue by using getKnownMinSize instead of the
implicit cast. This is possible because the code is already
scalable-vector aware. The semantic behaviour of the code is unchanged
by this patch.

[joechrisellis]

Accidentally changed int64_t to uint64_t in the last diff. Reverted this.

[joechrisellis]

Bail out early if we have scalable vectors.

[fpetrogalli]

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

803–809

I think you can do this:

if (dyn_cast<ScalableVectorType>(TargetType))
          return TTI::TCC_Basic;

You should also move this code inside the else branch below, right before computing ElementSize, because this would be one of the types that are not "StructType".

[joechrisellis]

Address fpetrogalli's suggestion.

[fpetrogalli]

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

815

I *think* that it is not a matter of "nicer". I think the comment should just say TODO: handle scalable vectors.

816

My bad (sorry): it seems that the LLVM coding standard is asking to replace dyn_cast with isa. It makes sense, given that the result of the dynamic cast is not used anywhere.

[joechrisellis]

Use isa<...> instead of dyn_cast<...>.

[sdesmalen]

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

787

If any of the indices is a scalable vector, then the result type will also be a scalable vector. So you can bail out at the start of the function.

[fpetrogalli]

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

787

My suggestion was to bail out in the place where the different types where handled, and where computation of the offset was done (the else after if (StructType)), so that we would have known where to start modifying the code for making BaseOffset able to support scalable vectors. I have accepted the patch, but feel free to follow Sander's suggestion if you prefer this early exit (which was your original code too IIRC!).

815

Nit: ... scalable vectors.

[joechrisellis]

Move scalable vector check to the top of the function as-per sdesmalen's suggestion.

[joechrisellis]

Use pointer element type to check if this is a scalar GEP.

[sdesmalen]

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

787

Okay, you can ignore my previous comment as I got confused between the indexed type being a scalable vector, and the index itself being a scalable vector. You'll indeed want to add the check in the loop itself, as the element type of a <vscale x 4 x i32>* is the the indexed type <vscale x 4 x i32>, for which the size is being queried. The code in the previous revision of the patch (https://reviews.llvm.org/D89872?id=299716) was correct.

[sdesmalen]

llvm/test/Analysis/CostModel/AArch64/gep-cost-scalable-vector-typesize-warning.ll

1 ↗

(On Diff #299918)

Sorry, just one more suggestion. You can test this more easily by invoking the cost-model directly on the GEP, like this:

RUN: opt -cost-model -analyze -mtriple=aarch64--linux-gnu -mattr=+sve 2>%t
RUN: FileCheck --check-prefix=WARN --allow-empty %s < %t

; WARN-NOT: warning: {{.*}}TypeSize is not scalable

define <vscale x 16 x i8>* @gep_scalable_vector(<vscale x 16 x i8>* %ptr) {
  %retval = getelementptr <vscale x 16 x i8>, <vscale x 16 x i8>* %ptr, i32 2
  ret <vscale x 16 x i8>* %retval
}

If you also add a CHECK line for the cost of the GEP itself, you can rename the test to cost-scalable-vector-gep.ll. We're about to start work on the vectorizer and the cost-model more broadly, so we'll be adding more similar(ly named) tests like this.

[joechrisellis]

Address sdesmalen's review comments.

• improve test.
• revert to old bail-out logic.

[sdesmalen]

LGTM! Thanks @joechrisellis

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

int getGEPCost(Type *PointeeType, const Value *Ptr,
                  TTI::TargetCostKind CostKind = TTI::TCK_SizeAndLatency) {
     assert(PointeeType && Ptr && "can't get GEPCost of nullptr");
     // TODO: will remove this when pointers have an opaque type.
     assert(Ptr->getType()->getScalarType()->getPointerElementType() ==
                PointeeType &&
            "explicit pointee type doesn't match operand's pointee type");
     auto *BaseGV = dyn_cast<GlobalValue>(Ptr->stripPointerCasts());
     bool HasBaseReg = (BaseGV == nullptr);
 
     auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());
     APInt BaseOffset(PtrSizeBits, 0);
     int64_t Scale = 0;
 
     auto GTI = gep_type_begin(PointeeType, Operands);
     Type *TargetType = nullptr;
 
     // Handle the case where the GEP instruction has a single operand,
     // the basis, therefore TargetType is a nullptr.
     if (Operands.empty())
       return !BaseGV ? TTI::TCC_Free : TTI::TCC_Basic;
 
     for (auto I = Operands.begin(); I != Operands.end(); ++I, ++GTI) {
       TargetType = GTI.getIndexedType();
       // We assume that the cost of Scalar GEP with constant index and the
       // cost of Vector GEP with splat constant index are the same.
       const ConstantInt *ConstIdx = dyn_cast<ConstantInt>(*I);
       if (!ConstIdx)
         if (auto Splat = getSplatValue(*I))
           ConstIdx = dyn_cast<ConstantInt>(Splat);
       if (StructType *STy = GTI.getStructTypeOrNull()) {
         // For structures the index is always splat or scalar constant
         assert(ConstIdx && "Unexpected GEP index");
         uint64_t Field = ConstIdx->getZExtValue();
         BaseOffset += DL.getStructLayout(STy)->getElementOffset(Field);
       } else {
-        int64_t ElementSize = DL.getTypeAllocSize(GTI.getIndexedType());
+        // If this operand is a scalable type, bail out early.
+        // TODO: handle scalable vectors
+        if (isa<ScalableVectorType>(TargetType))
+          return TTI::TCC_Basic;
+        int64_t ElementSize =
+            DL.getTypeAllocSize(GTI.getIndexedType()).getFixedSize();
         if (ConstIdx) {
           BaseOffset +=
               ConstIdx->getValue().sextOrTrunc(PtrSizeBits) * ElementSize;
         } else {
           // Needs scale register.
           if (Scale != 0)
             // No addressing mode takes two scale registers.
             return TTI::TCC_Basic;

llvm/test/Analysis/CostModel/AArch64/cost-scalable-vector-gep.ll

+; RUN: opt -cost-model -analyze -mtriple=aarch64--linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s < %t
+
+; This regression test is verifying that a GEP instruction performed on a
+; scalable vector does not produce a 'assumption that TypeSize is not scalable'
+; warning when performing cost analysis.
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+; CHECK: Cost Model: Found an estimated cost of 1 for instruction:   %retval = getelementptr
+define <vscale x 16 x i8>* @gep_scalable_vector(<vscale x 16 x i8>* %ptr) {
+  %retval = getelementptr <vscale x 16 x i8>, <vscale x 16 x i8>* %ptr, i32 2
+  ret <vscale x 16 x i8>* %retval
+}