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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:502
 
-  if (isa<ConstantArray>(C) || isa<ConstantVector>(C) ||
+  Constant *SplatVal =
+      isa<VectorType>(C->getType()) ? C->getSplatValue(false) : nullptr;
----------------
Can you split out all changes to make `ReadDataFromGlobal` work on scalable splats into a separate patch? I think the changes aren't trivial, and we want a fix for the assertion-failures in LLVM 14. The improvements to make this optimisation work for scalable vectors is less urgent.


================
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();
----------------
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());



================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:521
     for (; Index != NumElts; ++Index) {
-      if (!ReadDataFromGlobal(C->getAggregateElement(Index), Offset, CurPtr,
-                              BytesLeft, DL))
+      Constant *El = SplatVal ? SplatVal : C->getAggregateElement(Index);
+      if (!ReadDataFromGlobal(El, Offset, CurPtr, BytesLeft, DL))
----------------
Instead of checking for `SplatVal` and the special case for scalable vectors, I'd rather you just add this support to `Constant::getAggregateElement` directly.


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

https://reviews.llvm.org/D119238