[PATCH] D74740: AMDGPU/GlobalISel: Select G_SHUFFLE_VECTOR

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

LGTM but please consider the suggestions inline.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:53-54
+    return true;
+  return ((Mask[0] == 0 || Mask[0] == 1) && (Mask[1] == 0 || Mask[1] == 1)) ||
+         ((Mask[0] == 2 || Mask[0] == 3) && (Mask[1] == 2 || Mask[1] == 3));
+}
----------------
I realise it's a matter of taste but how about `(Mask[0] & 2) == (Mask[1] & 2)`?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:2125
+
+  if (isOneOrUndef(Mask[0]) && Mask[1] == -1) {
+    if (IsVALU) {
----------------
`Mask[0] == 1` might be clearer here, since we've already handled the all-undef case and we don't want to be handling it again here.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:2135
+    }
+  } else if (isZeroOrUndef(Mask[0]) && isZeroOrUndef(Mask[1])) {
+    if (IsVALU) {
----------------
`Mask[1] == 0` might be clearer. All other cases have been handled.

Don't you also want to separate out the `Mask[0] == -1 && Mask[1] == 0` case which can just be a left shift?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:2153
+    }
+  } else if (isOneOrUndef(Mask[0]) && isOneOrUndef(Mask[1])) {
+    if (IsVALU) {
----------------
`Mask[0] == 1 && Mask[1] == 1` might be clearer. All other case have been handled.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:2171
+    }
+  } else if (isOneOrUndef(Mask[0]) && isZeroOrUndef(Mask[1])) {
+    if (IsVALU) {
----------------
`Mask[0] == 1 && Mask[1] == 0` might be clearer. All other cases have been handled.


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

https://reviews.llvm.org/D74740