[AMDGPU] Avoid using s_cmpk when src0 is not register

[ruiling]

Summary

The hardware spec require src0 of s_cmpk should be a register. So, we
should not optimize s_cmp to s_cmpk if src0 is not register.

[arsenm]

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

6

positive checks are more useful. Also you can just generate these checks. Can you reproduce this with an IR test too?

[ruiling]

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

6

will try positive check, how to generate the checks? could you give a little bit more info? The original test case that hit the issue is over-complex I think. Normally, a constant expression at IR level is easy to be optimized off by the middle-end. so I think a .mir test is enough for this issue.

[ruiling]

auto-generate the checks in test

[ruiling]

ping?

[arsenm]

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

6

So what is the context this appears? Why wasn't it optimized out?

[nhaehnle]

Did you see @arsenm's comment?

[ruiling]

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

6

well I didn't carefully check the program yet to understand why the optimization algorithms in llvm fails to optimize the program. but I think that is another problem that worth a careful investigation. I will investigate and try to optimize it off later. But I think this patch can be merged, right? can anyone help to merge? I don't have commit access.

[ruiling]

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

6

@arsenm The issue occurs when running vulkancts again AMD open-source vulkan driver. I did a little more check, the test-case has ~40 BBs and lots of phi instructions which is later simplified and proved to be constant. And the problem may be because LLPC choose a subset of llvm optimization passes considering compilation time (https://github.com/GPUOpen-Drivers/llpc/blob/dev/lgc/patch/Patch.cpp#L223). I tried use LLVM standard set of passes, the constant was optimized off. I think the optimization passes for vulkan may need further tuning to reach a better trade-off between compile-time and quality of generated code.

[ruiling]

ping. can you help push the patch? @arsenm @nhaehnle

[foad]

In D83020#2146471, @ruiling wrote:

ping. can you help push the patch? @arsenm @nhaehnle

Do you have commit access? If not, I can commit this. See https://llvm.org/docs/DeveloperPolicy.html#id16 if you want to request access.

[ruiling]

@foad I don't have commit access, please go help push the patch if you think this is ok. I will apply commit access after some patches accepted. Thanks for pointing the useful link:)

[foad]

Committed in 1658b8d7ddb65eb78e1304b009f1043ab6d9463f (sorry I forgot to put a link to this review in the commit message).

llvm/lib/Target/AMDGPU/SIShrinkInstructions.cpp

 }
 
 static void shrinkScalarCompare(const SIInstrInfo *TII, MachineInstr &MI) {
   // cmpk instructions do scc = dst <cc op> imm16, so commute the instruction to
   // get constants on the RHS.
   if (!MI.getOperand(0).isReg())
     TII->commuteInstruction(MI, false, 0, 1);
 
+  // cmpk requires src0 to be a register
+  const MachineOperand &Src0 = MI.getOperand(0);
+  if (!Src0.isReg())
+    return;
+
   const MachineOperand &Src1 = MI.getOperand(1);
   if (!Src1.isImm())
     return;
 
   int SOPKOpc = AMDGPU::getSOPKOp(MI.getOpcode());
   if (SOPKOpc == -1)
     return;
 

llvm/test/CodeGen/AMDGPU/cmp_shrink.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass si-shrink-instructions -verify-machineinstrs %s -o - | FileCheck -check-prefix=GCN %s
+
+---
+name:             not_shrink_icmp
+body:             |
+  bb.0:
+    ; GCN-LABEL: name: not_shrink_icmp
+    ; GCN: S_CMP_GT_I32 1, 65, implicit-def $scc
+    S_CMP_GT_I32 1, 65, implicit-def $scc
+...