[PATCH] D98114: [Loads] Forward constant vector store to load of first element

Sat Apr 3 06:43:08 PDT 2021

nikic added inline comments.

================
Comment at: llvm/test/Transforms/InstCombine/load-store-forward.ll:33

+define i32 @vec_store_load_first_constexpr(i32* %p) {
+; CHECK-LABEL: @vec_store_load_first_constexpr(
----------------
bjope wrote:
> Maybe we should have some test cases with types when DataLayout::typeSizeEqualsStoreSize returns false. I actually think the result would be wrong here if for example using i4 (or i13) instead of i32, because we don't really know which bits that are loaded.
> 
> So there might be a bug in ConstantFoldLoadThroughBitcast, that it does not bail out if typeSizeEqualsStoreSize is false for the DestTy, that would be exposed by adding such test cases here.
> 
> (Maybe even typeAllocSize is of importance and not only typeStoreSize, or can we ignore alignment for the scalar data type?)
I've added a test in https://github.com/llvm/llvm-project/commit/1470f94d71c544327f76b85c55cb6f7cb43a6cbb, does that match what you had in mind?

After reading through https://llvm.org/docs/LangRef.html#vector-type I //think// the result is fine, but the description there is certainly confusing. For little-endian it should certainly be fine, and for big endian the result is inverted twice (once per the described element order and then again through endianness), so should also come out to the same result.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98114