[ConstantFold] use StoreSize for VectorType byte checking

[khei4]

Summary

While working on https://reviews.llvm.org/D150422, using Val.extractBits(8, n * 8).getZextValue() on ReadDataFromGlobal in ConstantFolding crashed in the vector-type awkward aligned case test.
Checking StoreSize on VectorType seems to match the TODO comment in tests.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

513

This fix is correct, but incomplete: We should also add a if (!DL.typeSizeEqualsStoreSize(EltTy)) return false; here. For non-byte-sized vectors the memory layout is packed, while this code will assume that there is padding to the next byte boundary between elements.

[khei4]

apply feedback and add test.

[khei4]

llvm/lib/Analysis/ConstantFolding.cpp

513

Thank you for the review! I added bailout and TODO comment and test for that!

[jsilvanus]

Thanks for the fix!

The test diff is good, we now pass the test correctly.
The code change looks reasonable to me, but I'm not very familiar with it (which is why I didn't fix it myself).

[nikic]

LGTM

llvm/test/Transforms/InstCombine/load-non-byte-sized-vector.ll

4 ↗

(On Diff #522044)

Nit: Seems like using decimal is simpler here :)

19 ↗

(On Diff #522044)

The first argument of the @report function doesn't really look necessary. Actually, it would be best to split this into two tests of the form

%ptr0 = getelementptr i8, ptr @foo, i64 0
%res0 = load i4, ptr %ptr0, align 1
ret i4 %res0

because this makes it easier for alive2 to verify than using function calls.

26 ↗

(On Diff #522044)

I'd suggest to add this test to the load.ll file. It doesn't seem worthwhile to have a separate test file for just this case.

[khei4]

apply feedbacks

[khei4]

@nikic Thank you for the review! (Rather than returning value twice, I shorten the global array and return the one.

llvm/test/Transforms/InstCombine/load-non-byte-sized-vector.ll

4 ↗

(On Diff #522044)

Thanks. Yeah, this was cargo cult coping from original byte cases ;)

19 ↗

(On Diff #522044)

because this makes it easier for alive2 to verify than using function calls.

Thank you! I mimic the recently viewed test cases, but on these few value cases, it looks reasonable!

19 ↗

(On Diff #522044)

Seems reasonable!

[khei4]

update broken test

llvm/lib/Analysis/ConstantFolding.cpp

while (true) {
       ByteOffset = 0;
       CurEltOffset = NextEltOffset;
     }
     // not reached.
   }
 
   if (isa<ConstantArray>(C) || isa<ConstantVector>(C) ||
       isa<ConstantDataSequential>(C)) {
-    uint64_t NumElts;
+    uint64_t NumElts, EltSize;
     Type *EltTy;
     if (auto *AT = dyn_cast<ArrayType>(C->getType())) {
       NumElts = AT->getNumElements();
       EltTy = AT->getElementType();
+      EltSize = DL.getTypeAllocSize(EltTy);
     } else {
       NumElts = cast<FixedVectorType>(C->getType())->getNumElements();
       EltTy = cast<FixedVectorType>(C->getType())->getElementType();
+      // TODO: For non-byte-sized vectors, current implementation assumes there is
+      // padding to the next byte boundary between elements.
+      if (!DL.typeSizeEqualsStoreSize(EltTy))
+        return false;
+
+      EltSize = DL.getTypeStoreSize(EltTy);
     }
-    uint64_t EltSize = DL.getTypeAllocSize(EltTy);
     uint64_t Index = ByteOffset / EltSize;
     uint64_t Offset = ByteOffset - Index * EltSize;
 
     for (; Index != NumElts; ++Index) {
       if (!ReadDataFromGlobal(C->getAggregateElement(Index), Offset, CurPtr,
                               BytesLeft, DL))
         return false;
 

llvm/test/Transforms/InstCombine/load-gep-overalign.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt                        -passes=instcombine -S %s | FileCheck %s --check-prefix=NATURAL
 ; RUN: opt --data-layout="i16:32" -passes=instcombine -S %s | FileCheck %s --check-prefix=OVERALIGNED
 
 ; The data layouts are little endian, so @foo is 0x0123456789ABCDEF in memory.
 @foo = constant <4 x i16> <i16 u0x2301, i16 u0x6745, i16 u0xAB89, i16 u0xEFCD>, align 8
 
 declare void @report(i64 %index, i8 %val)
 
 define void @test_vector_load_i8() {
 ; Access and report each individual byte in @foo.
 ; OVERALIGNED and NATURAL should have the same result, because the layout of vectors ignores
 ; element type alignment, and thus the representation of @foo is the same in both cases.
-;
-; TODO: The OVERALIGNED result is incorrect, as apparently padding bytes
-; are assumed as they would appear in an array. In vectors, there is no padding.
-;
 ; NATURAL-LABEL: @test_vector_load_i8(
 ; NATURAL-NEXT:    call void @report(i64 0, i8 1)
 ; NATURAL-NEXT:    call void @report(i64 1, i8 35)
 ; NATURAL-NEXT:    call void @report(i64 2, i8 69)
 ; NATURAL-NEXT:    call void @report(i64 3, i8 103)
 ; NATURAL-NEXT:    call void @report(i64 4, i8 -119)
 ; NATURAL-NEXT:    call void @report(i64 5, i8 -85)
 ; NATURAL-NEXT:    call void @report(i64 6, i8 -51)
 ; NATURAL-NEXT:    call void @report(i64 7, i8 -17)
 ; NATURAL-NEXT:    ret void
 ;
 ; OVERALIGNED-LABEL: @test_vector_load_i8(
 ; OVERALIGNED-NEXT:    call void @report(i64 0, i8 1)
 ; OVERALIGNED-NEXT:    call void @report(i64 1, i8 35)
-; OVERALIGNED-NEXT:    call void @report(i64 2, i8 0)
-; OVERALIGNED-NEXT:    call void @report(i64 3, i8 0)
-; OVERALIGNED-NEXT:    call void @report(i64 4, i8 69)
-; OVERALIGNED-NEXT:    call void @report(i64 5, i8 103)
-; OVERALIGNED-NEXT:    call void @report(i64 6, i8 0)
-; OVERALIGNED-NEXT:    call void @report(i64 7, i8 0)
+; OVERALIGNED-NEXT:    call void @report(i64 2, i8 69)
+; OVERALIGNED-NEXT:    call void @report(i64 3, i8 103)
+; OVERALIGNED-NEXT:    call void @report(i64 4, i8 -119)
+; OVERALIGNED-NEXT:    call void @report(i64 5, i8 -85)
+; OVERALIGNED-NEXT:    call void @report(i64 6, i8 -51)
+; OVERALIGNED-NEXT:    call void @report(i64 7, i8 -17)
 ; OVERALIGNED-NEXT:    ret void
 ;
   %ptr0 = getelementptr i8, ptr @foo, i64 0
   %res0 = load i8, ptr %ptr0, align 1
   call void @report(i64 0, i8 %res0)
 
   %ptr1 = getelementptr i8, ptr @foo, i64 1
   %res1 = load i8, ptr %ptr1, align 1

llvm/test/Transforms/InstCombine/load.ll

 ; CHECK-LABEL: @load_via_strip_invariant_group(
 ; CHECK-NEXT:    ret i32 37
 ;
   %a = call ptr @llvm.strip.invariant.group.p0(ptr @Y)
   %b = getelementptr i8, ptr %a, i64 8
   %d = load i32, ptr %b
   ret i32 %d
 }
+
+; TODO: For non-byte-sized vectors, current implementation assumes there is
+; padding to the next byte boundary between elements.
+@foo = constant <2 x i4> <i4 u0x1, i4 u0x2>, align 8
+
+define i4 @test_vector_load_i4_non_byte_sized() {
+; CHECK-LABEL: @test_vector_load_i4_non_byte_sized(
+; CHECK-NEXT:    [[RES0:%.*]] = load i4, ptr @foo, align 8
+; CHECK-NEXT:    ret i4 [[RES0]]
+;
+  %ptr0 = getelementptr i8, ptr @foo, i64 0
+  %res0 = load i4, ptr %ptr0, align 1
+  ret i4 %res0
+}