[PATCH] D74740: AMDGPU/GlobalISel: Select G_SHUFFLE_VECTOR
Jay Foad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 21 02:11:42 PST 2020
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
More information about the llvm-commits
mailing list