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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.
Herald added a project: All.


================
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))
----------------
sdesmalen wrote:
> Instead of checking for `SplatVal` and the special case for scalable vectors, I'd rather you just add this support to `Constant::getAggregateElement` directly.
Hi @sdesmalen, I'm not sure this is right to be honest. It seems like we already have separate Constant::getAggregateElement and Constant::getSplatValue functions for a reason. If I understand you correctly, what you're asking for is basically something that does:

  Constant *Constant::getAggregateElement(unsigned Elt) {
    if (Constant *SplatVal = getSplatValue(false))
      return SplatVal;
    .. existing code ..
  }

If we do this then doesn't it also make sense to simply get rid of getSplatValue and replace all uses with getAggregateElement, rather than have two differently named interfaces that do the same thing?


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

https://reviews.llvm.org/D119238