[PATCH] D127119: [SLP]Fix undef handling in gather function.

[Harald van Dijk via Phabricator via llvm-commits]

hvdijk added a comment.

Thanks, I am seeing improvements with this new version. I'll try to go over the changes in more detail later, some initial superficial comments now.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6756
+    return V;
+  }
+  template <typename U>
----------------
This extra overload does not look like it adds anything: it takes a parameter `U` if and only if `U` is `Value *`. The previous overload takes `Value *` if and only if `U` is `Value *`. That is the same thing; this new overload does not appear to add anything and the file compiles successfully for me if I remove this.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6761
+    return nullptr;
+  }
+  template <typename U>
----------------
This extra overload goes against the documentation above `ValueSelect`. The documentation for `ValueSelect` says it takes a `Value *`, and if a `Value *` is wanted, returns that value, otherwise returns a default value. This extra overload is needed because `ValueSelect::get` now gets called with a `const TreeEntry *` in the instantiation of `performExtractsShuffleAction<const TreeEntry>`, but that is contrary to its documentation and it is not clear what it is supposed to mean.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6797
+         (isUndefVector(Base) && isGuaranteedNotToBePoison(FirstV)))) ||
+      (!FirstV && !isUndefVector(Base));
   if (IsBaseNotUndef) {
----------------
`a && b || !a && c` is simpler expressed as `a ? b : c`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6807
+      } else {
+        IsCompleteIdentity &= (Res.second ? Idx : Mask[Idx]) == Idx;
         Mask[Idx] = (Res.second ? Idx : Mask[Idx]) + VF;
----------------
`(a ? b : c) == b` is simpler expressed as `a || c == b`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6813
+      // Found complete identity, i.e. elements only from single vector are
+      // selected.
+      Prev = Res.first;
----------------
This comment seems like it does not match how `IsCompleteIdentity` is set: we can get `IsCompleteIdentity` to be false even when we select elements only from a single vector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127119