[PATCH] D150515: [ConstantFold] use StoreSize for VectorType byte checking

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

@nikic Thank you for the review!



================
Comment at: llvm/test/Transforms/InstCombine/load-non-byte-sized-vector.ll:4
+
+ at foo = constant <4 x i4> <i4 u0x1, i4 u0x2, i4 u0x3, i4 u0x4>, align 8
+declare void @report(i64 %index, i4 %val)
----------------
nikic wrote:
> Nit: Seems like using decimal is simpler here :)
Thanks. Yeah, this was cargo cult coping from original byte cases ;)


================
Comment at: llvm/test/Transforms/InstCombine/load-non-byte-sized-vector.ll:19
+  %res0 = load i4, ptr %ptr0, align 1
+  call void @report(i64 0, i4 %res0)
+
----------------
nikic wrote:
> 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.
> 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!


================
Comment at: llvm/test/Transforms/InstCombine/load-non-byte-sized-vector.ll:19
+  %res0 = load i4, ptr %ptr0, align 1
+  call void @report(i64 0, i4 %res0)
+
----------------
khei4 wrote:
> nikic wrote:
> > 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.
> > 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!
Seems reasonable!


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

https://reviews.llvm.org/D150515