AMDGPU/GlobalISel: Allow selection of scalar min/max

[arsenm]

Summary

I believe all of the uniform/divergent pattern predicates are
redundant and can be removed. The uniformity bit already influences
the register class, and nothhing has broken when I've removed this and
others.

[alex-t]

lib/Target/AMDGPU/SOPInstructions.td

422

As you remove the "!isDiverent" predicate, I'd like to see the test that ensures that V_MIN/MAX etc are not selected for uniform smin/smax/umin/umax IR operations. The test should check not GlobalIsel but current instruction selection case.

[arsenm]

lib/Target/AMDGPU/SOPInstructions.td

422

Those should already exist

[alex-t]

lib/Target/AMDGPU/SOPInstructions.td

422

Yes. We have min.ll, max.ll and sminmax.ll
All except min.ll have "-amdgpu-scalarize-global-loads=false" flag in RUN line.
That means that they will select v_min/max always.
This was temporary solution a while ago to avoid massive change in the test base.
We need to remove that flags at least from max.ll and sminmax.ll and update the test until proceeding with current change.

Formally, guarding both VALU and SALU variants with same predicate looks extensive.
I have made small test and tried your change - it works as expected.

Nevertheless, I'd like to have clean test base until we remove odd predicates.
So, my idea is to create the change that removes "-amdgpu-scalarize-global-loads=false" from min/max tests. After submitting it you can safely go ahead.
Obviously I will try to submit the test change asap.

[alex-t]

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

[arsenm]

In D67596#1673887, @alex-t wrote:

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

The patterns are rejected for having custom predicate code. We don't need it as a standard pattern checks the register banks, which is what we really want. I think the cleanest compatibility solution would be to use divergence to guide register class selection, and somehow add the equivalent checks in the DAG patterns. Defining the equivalent GlobalISel pattern as always true would probably work, although leave a lot of clutter in the matching tables

[alex-t]

In D67596#1673947, @arsenm wrote:

In D67596#1673887, @alex-t wrote:

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

The patterns are rejected for having custom predicate code. We don't need it as a standard pattern checks the register banks, which is what we really want. I think the cleanest compatibility solution would be to use divergence to guide register class selection, and somehow add the equivalent checks in the DAG patterns. Defining the equivalent GlobalISel pattern as always true would probably work, although leave a lot of clutter in the matching tables

Sounds good but I'd like to keep existing stuff working until we get ready to switch to the new solution.

[alex-t]

LGTM but make sure it passes recently updated lit tests.

[arsenm]

r372450

lib/Target/AMDGPU/SOPInstructions.td

 
 def S_SUBB_U32 : SOP2_32 <"s_subb_u32",
   [(set i32:$sdst, (UniformBinFrag<sube> (i32 SSrc_b32:$src0), (i32 SSrc_b32:$src1)))]>;
 } // End Uses = [SCC]
 
 
 let isCommutable = 1 in {
 def S_MIN_I32 : SOP2_32 <"s_min_i32",
-  [(set i32:$sdst, (UniformBinFrag<smin> i32:$src0, i32:$src1))]
+  [(set i32:$sdst, (smin i32:$src0, i32:$src1))]
 >;
 def S_MIN_U32 : SOP2_32 <"s_min_u32",
-  [(set i32:$sdst, (UniformBinFrag<umin> i32:$src0, i32:$src1))]
+  [(set i32:$sdst, (umin i32:$src0, i32:$src1))]
 >;
 def S_MAX_I32 : SOP2_32 <"s_max_i32",
-  [(set i32:$sdst, (UniformBinFrag<smax> i32:$src0, i32:$src1))]
+  [(set i32:$sdst, (smax i32:$src0, i32:$src1))]
 >;
 def S_MAX_U32 : SOP2_32 <"s_max_u32",
-  [(set i32:$sdst, (UniformBinFrag<umax> i32:$src0, i32:$src1))]
+  [(set i32:$sdst, (umax i32:$src0, i32:$src1))]
 >;
 } // End isCommutable = 1
 } // End Defs = [SCC]
 
 
 let Uses = [SCC] in {
   def S_CSELECT_B32 : SOP2_32 <"s_cselect_b32">;
   def S_CSELECT_B64 : SOP2_64 <"s_cselect_b64">;

test/CodeGen/AMDGPU/GlobalISel/inst-select-smax.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2>%t | FileCheck -check-prefix=GCN %s
-# RUN: FileCheck -check-prefix=ERR  %s < %t
-
-# ERR-NOT: remark:
-# ERR: remark: <unknown>:0:0: cannot select: %2:sgpr(s32) = G_SMAX %0:sgpr, %1:sgpr (in function: smax_s32_ss)
-# ERR-NOT: remark:
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck -check-prefix=GCN %s
 
 ---
 name: smax_s32_ss
 legalized: true
 regBankSelected: true
 
 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1
     ; GCN-LABEL: name: smax_s32_ss
-    ; GCN: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
-    ; GCN: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr1
-    ; GCN: [[SMAX:%[0-9]+]]:sgpr(s32) = G_SMAX [[COPY]], [[COPY1]]
-    ; GCN: S_ENDPGM 0, implicit [[SMAX]](s32)
+    ; GCN: [[COPY:%[0-9]+]]:sreg_32 = COPY $sgpr0
+    ; GCN: [[COPY1:%[0-9]+]]:sreg_32 = COPY $sgpr1
+    ; GCN: [[S_MAX_I32_:%[0-9]+]]:sreg_32 = S_MAX_I32 [[COPY]], [[COPY1]], implicit-def $scc
+    ; GCN: S_ENDPGM 0, implicit [[S_MAX_I32_]]
     %0:sgpr(s32) = COPY $sgpr0
     %1:sgpr(s32) = COPY $sgpr1
     %2:sgpr(s32) = G_SMAX %0, %1
     S_ENDPGM 0, implicit %2
 ...
 
 ---
 name: smax_s32_sv

test/CodeGen/AMDGPU/GlobalISel/inst-select-smin.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2>%t | FileCheck -check-prefix=GCN %s
-# RUN: FileCheck -check-prefix=ERR  %s < %t
-
-# ERR-NOT: remark:
-# ERR: remark: <unknown>:0:0: cannot select: %2:sgpr(s32) = G_SMIN %0:sgpr, %1:sgpr (in function: smin_s32_ss)
-# ERR-NOT: remark:
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -verify-machineinstrs %s -o -  | FileCheck -check-prefix=GCN %s
 
 ---
 name: smin_s32_ss
 legalized: true
 regBankSelected: true
 
 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1
     ; GCN-LABEL: name: smin_s32_ss
-    ; GCN: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
-    ; GCN: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr1
-    ; GCN: [[SMIN:%[0-9]+]]:sgpr(s32) = G_SMIN [[COPY]], [[COPY1]]
-    ; GCN: S_ENDPGM 0, implicit [[SMIN]](s32)
+    ; GCN: [[COPY:%[0-9]+]]:sreg_32 = COPY $sgpr0
+    ; GCN: [[COPY1:%[0-9]+]]:sreg_32 = COPY $sgpr1
+    ; GCN: [[S_MIN_I32_:%[0-9]+]]:sreg_32 = S_MIN_I32 [[COPY]], [[COPY1]], implicit-def $scc
+    ; GCN: S_ENDPGM 0, implicit [[S_MIN_I32_]]
     %0:sgpr(s32) = COPY $sgpr0
     %1:sgpr(s32) = COPY $sgpr1
     %2:sgpr(s32) = G_SMIN %0, %1
     S_ENDPGM 0, implicit %2
 ...
 
 ---
 name: smin_s32_sv

test/CodeGen/AMDGPU/GlobalISel/inst-select-umax.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2>%t | FileCheck -check-prefix=GCN %s
-# RUN: FileCheck -check-prefix=ERR  %s < %t
-
-# ERR-NOT: remark:
-# ERR: remark: <unknown>:0:0: cannot select: %2:sgpr(s32) = G_UMAX %0:sgpr, %1:sgpr (in function: umax_s32_ss)
-# ERR-NOT: remark:
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck -check-prefix=GCN %s
 
 ---
 name: umax_s32_ss
 legalized: true
 regBankSelected: true
 
 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1
     ; GCN-LABEL: name: umax_s32_ss
-    ; GCN: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
-    ; GCN: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr1
-    ; GCN: [[UMAX:%[0-9]+]]:sgpr(s32) = G_UMAX [[COPY]], [[COPY1]]
-    ; GCN: S_ENDPGM 0, implicit [[UMAX]](s32)
+    ; GCN: [[COPY:%[0-9]+]]:sreg_32 = COPY $sgpr0
+    ; GCN: [[COPY1:%[0-9]+]]:sreg_32 = COPY $sgpr1
+    ; GCN: [[S_MAX_U32_:%[0-9]+]]:sreg_32 = S_MAX_U32 [[COPY]], [[COPY1]], implicit-def $scc
+    ; GCN: S_ENDPGM 0, implicit [[S_MAX_U32_]]
     %0:sgpr(s32) = COPY $sgpr0
     %1:sgpr(s32) = COPY $sgpr1
     %2:sgpr(s32) = G_UMAX %0, %1
     S_ENDPGM 0, implicit %2
 ...
 
 ---
 name: umax_s32_sv

test/CodeGen/AMDGPU/GlobalISel/inst-select-umin.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2>%t | FileCheck -check-prefix=GCN %s
-# RUN: FileCheck -check-prefix=ERR  %s < %t
-
-# ERR-NOT: remark:
-# ERR: remark: <unknown>:0:0: cannot select: %2:sgpr(s32) = G_UMIN %0:sgpr, %1:sgpr (in function: umin_s32_ss)
-# ERR-NOT: remark:
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=instruction-select -verify-machineinstrs %s -o - 2>%t | FileCheck -check-prefix=GCN %s
 
 ---
 name: umin_s32_ss
 legalized: true
 regBankSelected: true
 
 body: |
   bb.0:
     liveins: $sgpr0, $sgpr1
     ; GCN-LABEL: name: umin_s32_ss
-    ; GCN: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
-    ; GCN: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr1
-    ; GCN: [[UMIN:%[0-9]+]]:sgpr(s32) = G_UMIN [[COPY]], [[COPY1]]
-    ; GCN: S_ENDPGM 0, implicit [[UMIN]](s32)
+    ; GCN: [[COPY:%[0-9]+]]:sreg_32 = COPY $sgpr0
+    ; GCN: [[COPY1:%[0-9]+]]:sreg_32 = COPY $sgpr1
+    ; GCN: [[S_MIN_U32_:%[0-9]+]]:sreg_32 = S_MIN_U32 [[COPY]], [[COPY1]], implicit-def $scc
+    ; GCN: S_ENDPGM 0, implicit [[S_MIN_U32_]]
     %0:sgpr(s32) = COPY $sgpr0
     %1:sgpr(s32) = COPY $sgpr1
     %2:sgpr(s32) = G_UMIN %0, %1
     S_ENDPGM 0, implicit %2
 ...
 
 ---
 name: umin_s32_sv