[PATCH] D24265: AMDGPU: Use SOPK compare instructions

[Nicolai Hähnle via llvm-commits]

nhaehnle added a comment.

Mostly looks good to me, but one question and I think there is a bug, see inline.


================
Comment at: lib/Target/AMDGPU/SIShrinkInstructions.cpp:245-246
@@ +244,4 @@
+
+  // eq/ne is special because the imm16 can be treated as signed or unsigned.
+  if (SOPKOpc == AMDGPU::S_CMPK_EQ_I32 || SOPKOpc == AMDGPU::S_CMPK_LG_I32) {
+    bool HasUImm;
----------------
Why isn't there similar treatment for SOPKOpc == AMDGPU::S_CMPK_EQ_U32 or ..._LG_U32? Are only the I32 variants selected previously? If so, that should probably be mentioned in the comment.

================
Comment at: lib/Target/AMDGPU/SIShrinkInstructions.cpp:261-264
@@ +260,6 @@
+  const MCInstrDesc &NewDesc = TII->get(SOPKOpc);
+  if ((TII->sopkIsZext(SOPKOpc) && isKUImmOperand(TII, Src1)) ||
+      isKImmOperand(TII, Src1)) {
+    MI.setDesc(NewDesc);
+  }
+}
----------------
I think you need a !TII->sopkIsZext test to go with the isKImmOperand test (consider what happens if you get s_cmp_lt_u32 op1, -1).

================
Comment at: test/CodeGen/AMDGPU/sopk-compares.ll:335-340
@@ +334,8 @@
+
+; GCN-LABEL: {{^}}br_scc_ult_i32_min_simm16:
+; GCN: s_cmpk_lt_u32 s{{[0-9]+}}, 0x8000{{$}}
+define void @br_scc_ult_i32_min_simm16(i32 %cond, i32 addrspace(1)* %out) #0 {
+entry:
+  %cmp0 = icmp ult i32 %cond, -32768
+  br i1 %cmp0, label %endif, label %if
+
----------------
This test looks wrong to me. -32768 is 0xffff8000, but s_cmpk_lt_u32 will not sign-extend.


https://reviews.llvm.org/D24265