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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope 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() ||
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76944