[GVN] Fix VNCoercion for Scalable Vector.

[huihuiz]

Summary

For VNCoercion, skip scalable vector when analysis rely on fixed size,
otherwise call TypeSize::getFixedSize() explicitly.

Add unit tests to check funtionality of GVN load elimination for scalable type.

[efriedma]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

543 ↗

(On Diff #253187)

Can you check DL.getTypeAllocSize(GEPIdxedTy).Scalable instead, since you're calling getTypeAllocSize anyway?

I don't think this is the right way to break out of this loop early; if the offset isn't recorded in "Decomposed", other code might assume it's zero?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

1880

I think TypeSize also has isZero now, assuming that got merged.

llvm/lib/Transforms/Utils/VNCoercion.cpp

24–25

Fix the comment here?

Can you refactor the whole Ty->isStructTy() || LoadTy->isArrayTy() || (Ty->isVectorTy() && Ty->getVectorIsScalable()) thing into a helper function, so the overall property has a name?

[bjope]

llvm/lib/Transforms/Utils/VNCoercion.cpp

24–25

I agree that refactoring this check into a helper could be wise.

Just out of curiosity, shouldn't we also check DL.typeSizeEuqalsStoreSize in such a helper?

Background for that question is that I've been investigating some problems downstream (where we got some types that aren't byte sized and thus include paddning in memory).

Consider this example (using types available upstream):

define <4 x i1> @v4i8_to_v4i1() {
  %tmp = alloca <4 x i8>, align 1
  store <4 x i8> <i8 4, i8 -1, i8 -1, i8 -1>, <4 x i8>* %tmp, align 1
  %sroa_cast = bitcast <4 x i8>* %tmp to <4 x i1>*
  %dst = load <4 x i1>, <4 x i1>* %sroa_cast, align 1
  ret <4 x i1> %dst
}

And notice that the returned value will differ depending on if we have big/little endian:

> opt -gvn -S -data-layout "e" gvn-test.ll
; ModuleID = 'gvn-test.ll'
source_filename = "gvn-test.ll"
target datalayout = "e"

define <4 x i1> @v4i8_to_v4i1() {
  %tmp = alloca <4 x i8>, align 1
  store <4 x i8> <i8 4, i8 -1, i8 -1, i8 -1>, <4 x i8>* %tmp, align 1
  %sroa_cast = bitcast <4 x i8>* %tmp to <4 x i1>*
  ret <4 x i1> <i1 false, i1 false, i1 true, i1 false>
}

> opt -gvn -S -data-layout "E" gvn-test.ll

; ModuleID = 'gvn-test.ll'
source_filename = "gvn-test.ll"
target datalayout = "E"

define <4 x i1> @v4i8_to_v4i1() {
  %tmp = alloca <4 x i8>, align 1
  store <4 x i8> <i8 4, i8 -1, i8 -1, i8 -1>, <4 x i8>* %tmp, align 1
  %sroa_cast = bitcast <4 x i8>* %tmp to <4 x i1>*
  ret <4 x i1> <i1 false, i1 true, i1 false, i1 false>
}

So we might wanna avoid VNCoercion for types with DL.typeSizeEuqalsStoreSize(Ty) being false. At least for vector types. For scalars the alignTo check a few lines down offer some protection, but maybe checking DL.typeSizeEuqalsStoreSize(Ty) make sense for scalars as well. Or what do you think?

[bjope]

llvm/lib/Transforms/Utils/VNCoercion.cpp

24–25

Well, it is not wrong that we get different result for big/little endian in my example above. Just that it is unclear to my where the bits go when storing <4 x i1> to memory. Is it well defined where the four bits go when storing <4 x i1> to memory?

[efriedma]

llvm/lib/Transforms/Utils/VNCoercion.cpp

24–25

Yes, there's a consistent rule that should be used everywhere. Maybe the simplest description is just the code in foldConstVectorToAPInt in ConstantFolding.cpp.

[huihuiz]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

543 ↗

(On Diff #253187)

Spend sometime look again, BasicAAResult::aliasGEP is using the offset/varIndices recorded in DecomposedGEP. For scalable type, we might end up inaccurate alias result.

Working on fixes for this logic.

[huihuiz]

Address review comments.

[huihuiz]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

543 ↗

(On Diff #253187)

Fixing BasicAA in D77828.

[efriedma]

LGTM