[PATCH] D76944: [GVN] Fix VNCoercion/BasicAA for Scalable Vector.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/VNCoercion.cpp:21
   // If the loaded or stored value is an first class array or struct, don't try
   // to transform them.  We need to be able to bitcast to integer.
   if (LoadTy->isStructTy() || LoadTy->isArrayTy() || StoredTy->isStructTy() ||
----------------
bjope wrote:
> bjope wrote:
> > efriedma wrote:
> > > 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?
> > 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?
> 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?
Yes, there's a consistent rule that should be used everywhere.  Maybe the simplest description is just the code in foldConstVectorToAPInt in ConstantFolding.cpp.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76944/new/

https://reviews.llvm.org/D76944