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

[David Sherwood via Phabricator via llvm-commits]

david-arm marked an inline comment as done.
david-arm 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);
----------------
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?


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

https://reviews.llvm.org/D119238