[PATCH] D130487: [PowerPC] Fix vector_shuffle combines when inputs are scalar_to_vector of differing types.

[Kamau Bridgeman via Phabricator via llvm-commits]

kamaub requested changes to this revision.
kamaub added a comment.
This revision now requires changes to proceed.

Request changes because of the bug in the `isShuffleMaskInRange()` conditions



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14992
+                                 int RHSLastByteDefined) {
+  for (int i = 0, elt = ShuffV.size(); i < elt; i++) {
+    int Index = ShuffV[i];
----------------
please use a range based loop


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14997-14998
+    // Handle first input vector of the vector_shuffle.
+    if (Index < HalfVec && LHSLastByteDefined >= 0) {
+      if (!(Index <= LHSLastByteDefined))
+        return false;
----------------
nemanjai wrote:
> Why do we only check here if `LHSLastByteDefined >= 0` and similarly for the RHS below? Do we really want to pretend that a shuffle mask is in range if the last byte/element defined is undefined (i.e. presumably no bytes are defined)?



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14998
+    if (Index < HalfVec && LHSLastByteDefined >= 0) {
+      if (!(Index <= LHSLastByteDefined))
+        return false;
----------------
Please change this to `>` (and for below).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15002-15003
+    // Handle second input vector of the vector_shuffle.
+    if (Index >= HalfVec && RHSLastByteDefined >= 0) {
+      if (!(Index <= Index + (RHSLastByteDefined)))
+        return false;
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15070
     // Initially assume that neither input is permuted. These will be adjusted
     // accordingly if either input is.
+    int LHSFirstElt = 0;
----------------
please expand this comment to point out that -1 means all elements are defined


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130487