[PATCH] D115726: [InstCombine] Fold for masked gather when loading the same value each time.

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This looks like a nice change @CarolineConcatto! I just have a few comments about the tests.



================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:4
+
+define <vscale x 2 x i64> @valid_inv_load_i64(i64* %src) #0 {
+; CHECK-LABEL: @valid_inv_load_i64(
----------------
nit: Could you rename this to `@valid_invariant_load_i64` just because the shortened form `inv` could also mean `invalid`?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:18
+;; Not a splat value
+define <vscale x 2 x i64> @invalid_value_inv_load_i64(i64* %src) #0 {
+; CHECK-LABEL: @invalid_value_inv_load_i64(
----------------
nit: Same here - perhaps rename to `@invalid_value_invariant_load_i64`?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:26
+  %broadcast.splatinsert = insertelement <vscale x 2 x i64*> poison, i64 *%src, i32 1
+  %broadcast.splat = shufflevector <vscale x 2 x i64*> %broadcast.splatinsert, <vscale x 2 x i64*> poison, <vscale x 2 x i32> zeroinitializer
+  %res = call <vscale x 2 x i64> @llvm.masked.gather.nxv2i64(<vscale x 2 x i64*> %broadcast.splat, i32 8, <vscale x 2 x i1> shufflevector (<vscale x 2 x i1> insertelement (<vscale x 2 x i1> poison, i1 true, i32 0), <vscale x 2 x i1> poison, <vscale x 2 x i32> zeroinitializer), <vscale x 2 x i64> undef)
----------------
This is actually just creating a fully poison vector here because you're splatting the first element, which is poison. Perhaps you can just delete the `%broadcast.splat` line and pass `%broadcast.splatinsert` directly to the `masked.gather`?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:32
+;; Not all one mask
+define <vscale x 2 x i64> @invalid_mask_inv_load_i64(i64* %src) #0 {
+; CHECK-LABEL: @invalid_mask_inv_load_i64(
----------------
nit: Rename to something like `@invalid_mask_invariant_load_i64`?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:41
+  %broadcast.splat = shufflevector <vscale x 2 x i64*> %broadcast.splatinsert, <vscale x 2 x i64*> poison, <vscale x 2 x i32> zeroinitializer
+  %res = call <vscale x 2 x i64> @llvm.masked.gather.nxv2i64(<vscale x 2 x i64*> %broadcast.splat, i32 8, <vscale x 2 x i1> shufflevector (<vscale x 2 x i1> insertelement (<vscale x 2 x i1> poison, i1 true, i32 1), <vscale x 2 x i1> poison, <vscale x 2 x i32> zeroinitializer), <vscale x 2 x i64> undef)
+  ret <vscale x 2 x i64> %res
----------------
This is equivalent to a fully `poison` vector because you're splatting the first element, which is poison. Perhaps you can just pass a predicate to the function:

  define <vscale x 2 x i64> @invalid_mask_invariant_load_i64(i64* %src, <vscale x 2 x i1> %mask) #0 {
    ...
    %res = call <vscale x 2 x i64> @llvm.masked.gather.nxv2i64(<vscale x 2 x i64*> %broadcast.splat, i32 8, <vscale x 2 x i1> %mask, <vscale x 2 x i64> undef)

?

Alternatively, if you want to test for a constant mask that isn't all zeroes you can do something like:

  %res = call <vscale x 2 x i64> @llvm.masked.gather.nxv2i64(<vscale x 2 x i64*> %broadcast.splat, i32 8, <vscale x 2 x i1> shufflevector (<vscale x 2 x i1> insertelement (<vscale x 2 x i1> poison, i1 false, i32 0), <vscale x 2 x i1> poison, <vscale x 2 x i32> zeroinitializer), <vscale x 2 x i64> undef)

which is an all-false constant mask?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115726