[PATCH] D119238: [InstCombine] Support load-store forwarding with scalable vector types

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:512-513
     } else {
-      NumElts = cast<FixedVectorType>(C->getType())->getNumElements();
-      EltTy = cast<FixedVectorType>(C->getType())->getElementType();
+      NumElts =
+          cast<VectorType>(C->getType())->getElementCount().getKnownMinValue();
+      EltTy = cast<VectorType>(C->getType())->getElementType();
----------------
sdesmalen wrote:
> I'd really like to avoid things like this, where a variable is named `NumElts` and we use the "Minimum known number of elements" from a scalable vector.
> 
> To avoid relying on the VectorType's ElementCount entirely, you can define the number of elements based on `BytesLeft`, i.e.:
> 
>   Type *EltTy;
>   if (auto *AT = dyn_cast<ArrayType>(C->getType()))
>     EltTy = AT->getElementType();
>   else
>     EltTy = cast<VectorType>(C->getType())->getElementType();
> 
>   uint64_t NumElts =
>       std::max<uint64_t>(1, (8 * BytesLeft) / EltTy->getScalarSizeInBits());
> 
So the problem with this approach is that currently `BytesLeft` can be larger than the type you're reading from. For example, in test/Transforms/InstCombine/strncpy-3.ll we try to read 4 bytes from an array of 3 bytes. In your example code above that means NumElts would be 4 and we end up crashing. That means we still have to use the number of elements in the type to limit the reads/writes to only those that are valid. Perhaps in the first instance where we call `ReadDataFromGlobal` I can clamp the value of BytesLeft to the known minimum size of the type?


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

https://reviews.llvm.org/D119238