Page MenuHomePhabricator

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

Authored by joechrisellis on Oct 21 2020, 4:34 AM.

Details

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.

Diff Detail

Event Timeline

joechrisellis created this revision.Oct 21 2020, 4:34 AM
joechrisellis requested review of this revision.Oct 21 2020, 4:34 AM

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

Bail out early if we have scalable vectors.

fpetrogalli added inline comments.Oct 21 2020, 7:53 AM
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".

Address fpetrogalli's suggestion.

fpetrogalli added inline comments.Oct 21 2020, 8:40 AM
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.

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

joechrisellis marked an inline comment as done.Oct 21 2020, 9:14 AM
sdesmalen added inline comments.Oct 21 2020, 9:18 AM
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 accepted this revision.Oct 22 2020, 12:53 AM
fpetrogalli added inline comments.
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.

This revision is now accepted and ready to land.Oct 22 2020, 12:53 AM
joechrisellis marked an inline comment as done.

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

joechrisellis marked an inline comment as done.Oct 22 2020, 3:12 AM

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

sdesmalen added inline comments.Oct 23 2020, 11:09 AM
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 added inline comments.Oct 26 2020, 2:32 AM
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.

Address sdesmalen's review comments.

  • improve test.
  • revert to old bail-out logic.
This revision was landed with ongoing or failed builds.Oct 26 2020, 10:41 AM
This revision was automatically updated to reflect the committed changes.