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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @CarolineConcatto, the changes looks good to me, just left a few more nits on the tests.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:370
+      LoadInst *L = Builder.CreateAlignedLoad(VecTy->getElementType(), SplatPtr,
+                                              Alignment, "load.combine");
+      Value *Shuf =
----------------
nit: `load.scalar` ?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -instcombine -S < %s | FileCheck %s
----------------
nit: From what I can see, there is nothing really scalable-specific about this transformation, so maybe just add it to llvm/test/Transforms/InstCombine/masked_intrinsics.ll?
(and also add a test for fixed-width vectors)


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:5
+;; Splat Value and all active/one mask
+define <vscale x 2 x i64> @valid_invariant_load_i64(i64* %src) #0 {
+; CHECK-LABEL: @valid_invariant_load_i64(
----------------
nit: Did you mean `uniform`?

I'd also avoid using 'valid' and 'invalid', because all IR seems valid, it's just not necessarily the right input to trigger your transformation.

How about:

  @gather_nxv2i64_uniform_ptrs_all_active_mask
  @gather_nxv2i64_non_uniform_ptrs_all_active_mask
  @gather_nxv2i64_uniform_ptrs_no_all_active_mask


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:18
+
+;; Not a splat value
+define <vscale x 2 x i64> @invalid_value_invariant_load_i64(i64* %src ) #0 {
----------------
nit: Vector of pointers is not a splat value.


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:25
+;
+  %insert.value = insertelement <vscale x 2 x i64*> poison, i64 *%src, i32 1
+  %res = call <vscale x 2 x i64> @llvm.masked.gather.nxv2i64(<vscale x 2 x i64*> %insert.value, 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)
----------------
nit: maybe just pass a `<vscale x 2 x i64*>` as argument?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_masked_intrinsics.ll:30
+
+;; Not all active/one or inactive/zero mask
+define <vscale x 2 x i64> @invalid_mask_invariant_load_i64(i64* %src, <vscale x 2 x i1> %mask) #0 {
----------------
nit: Do you just mean 'Not all active mask' ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115726