This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Fix VNCoercion for Scalable Vector.
ClosedPublic

Authored by huihuiz on Mar 27 2020, 11:59 AM.

Details

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.

Diff Detail

Event Timeline

huihuiz created this revision.Mar 27 2020, 11:59 AM
efriedma added inline comments.Mar 27 2020, 12:16 PM
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 added a subscriber: bjope.Mar 28 2020, 10:29 AM
bjope added inline comments.
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 added inline comments.Mar 28 2020, 10:38 AM
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 added inline comments.Mar 28 2020, 2:57 PM
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 added inline comments.Mar 30 2020, 9:56 AM
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 updated this revision to Diff 256446.Apr 9 2020, 4:42 PM
huihuiz marked 3 inline comments as done.

Address review comments.

huihuiz retitled this revision from [GVN] Fix VNCoercion/BasicAA for Scalable Vector. to [GVN] Fix VNCoercion for Scalable Vector..Apr 9 2020, 4:43 PM
huihuiz edited the summary of this revision. (Show Details)
huihuiz added inline comments.
llvm/lib/Analysis/BasicAliasAnalysis.cpp
543 ↗(On Diff #253187)

Fixing BasicAA in D77828.

efriedma accepted this revision.Apr 9 2020, 5:22 PM

LGTM

This revision is now accepted and ready to land.Apr 9 2020, 5:22 PM
This revision was automatically updated to reflect the committed changes.