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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:602
   // If we're not accessing anything in this constant, the result is undefined.
-  if (Offset >= InitializerSize)
+  if (Offset >= static_cast<int64_t>(InitializerSize.getKnownMinValue()))
     return UndefValue::get(IntType);
----------------
david-arm wrote:
> nikic wrote:
> > david-arm wrote:
> > > nikic wrote:
> > > > This is saying that accessing a scalable vector past its *minimum* size is undefined behavior. This doesn't look right to me. You probably want to return nullptr in that case, not UndefValue.
> > > Well spotted, thanks @nikic !
> > I think the code in ReadDataFromGlobal suffers from a similar issue, where a read that starts before the min size but crossed over it will leave the remaining bytes as zero.
> I couldn't seem to create a test for that because whenever the loaded type is larger than the known minimum size of the constant type we don't perform the optimisation at all. Also, I wasn't able to construct a test case where the start offset is non-zero because then we don't even get into this function (I tried using getelementptr to load from an offset of the store ptr). If you have any suggestions on how to this cross-over case I'm happy to add them? Alternatively, I could just add an assert for that case?
Are scalable vectors supported as globals? If so, the more straightforward way to test this code is to perform a load from a scalable vector constant, in which case you can also test GEP with offset. The InstCombine code you're targeting currently only allows zero offsets.


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

https://reviews.llvm.org/D119238