[AMDGPU] Fix size of SOPK instructions to 4 bytes

[gandhi21299]

Summary

Instructions in SOPK format may not have 32-bit
literal constants following the instruction.

[Joe_Nash]

I believe you are correct about SOPK being fixed size. This looks like it could be an NFC change, because the code below would detect if there was a literal used. If you have any reason to think its not NFC, please add a test. That said, I think a better way to do the change is set FixedSize = 1 on SOPP_Pseudo.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7486 ↗

(On Diff #460498)

Shouldn't special case this here, FixedSize should be set.

[gandhi21299]

Set FixedSize for SOPK_Pseudo

[gandhi21299]

@Joe_Nash This patch fixes a bug where a literal is appended to the instruction. It has hardly been observed, however a test will be helpful. I will write one up,

[foad]

In D133972#3793361, @Joe_Nash wrote:

I believe you are correct about SOPK being fixed size. This looks like it could be an NFC change, because the code below would detect if there was a literal used. If you have any reason to think its not NFC, please add a test. That said, I think a better way to do the change is set FixedSize = 1 on SOPP_Pseudo.

Right, FixedSize was originally introduced (D25500) as a special case for instructions that are known not to have a literal operand even though the InOperandList contains something unusual that might look like a literal. It should not be needed in general for all instructions that happen to have a fixed size. So it would be nice to know what unusual case prompted this patch.

[gandhi21299]

• added a test to check for block sizes through offsets printed during branch relaxation

[arsenm]

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

2

If you're going to rely on -debug-only, this needs REQUIRES: asserts

12

Don't need attrs comment

13

I'm not seeing where there's a sopk instruction in this function

90–91

Don't need all the instruction flags. Should also use named values

[gandhi21299]

Address Matt's comments

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

13

s_waitcnt_* are all SOPK instructions

90–91

I will remove them since they are eliminated and do not generate any SOPK anyways.

[arsenm]

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

6–10

For a function that does not require relaxation, how did it introduce a block with size 0?

[gandhi21299]

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

6–10

It was first introduced due to the structurize CFG pass. I think it combines a sequence of instructions into a phi instruction.

[arsenm]

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

6–10

I'd somewhat prefer if the IR was a more direct match to what comes out here, so pre-structurize. You could also try using s.setreg intrinsics for direct emission of a sopk

[gandhi21299]

• Simplified sopk-no-literal.ll test. This test checks for the size of the kernel which should be 28 bytes as each instruction is 4 bytes long, except for s_load_b32 which is 8 bytes in size.

[gandhi21299]

• Adding a newline in the end of the test file.

llvm/lib/Target/AMDGPU/SOPInstructions.td

class SOPK_Pseudo <string opName, dag outs, dag ins,
   SIMCInstr<opName, SIEncodingFamily.NONE> {
   let isPseudo = 1;
   let isCodeGenOnly = 1;
   let mayLoad = 0;
   let mayStore = 0;
   let hasSideEffects = 0;
   let SALU = 1;
   let SOPK = 1;
+  let FixedSize = 1;
   let SchedRW = [WriteSALU];
   let UseNamedOperandTable = 1;
   string Mnemonic = opName;
   string AsmOperands = asmOps;
 
   bits<1> has_sdst = 1;
 }
 

llvm/test/CodeGen/AMDGPU/sopk-no-literal.ll

+; REQUIRES: asserts
+; RUN: llc -O0 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1100 -debug-only=branch-relaxation -verify-machineinstrs < %s 2>&1 | FileCheck --check-prefix=GFX10 %s
+
+; GFX10: Basic blocks after relaxation
+; GFX10: %bb.0  offset=00000000 size=0x1c
+
+; Each instruction in the following kernel is 4 bytes in size,
+; except s_load_b32 which is 8 bytes in size. Hence, 0x1c bytes in total.
+define amdgpu_kernel void @test_sopk_size(i32 %var.mode) {
+; GFX10-LABEL: test_sopk_size:
+; GFX10:  ; %bb.0:
+; GFX10:    s_load_b32 s0, s[0:1], 0x0
+; GFX10:    s_mov_b32 s1, 3
+; GFX10:    s_setreg_b32 hwreg(HW_REG_MODE, 0, 2), s1
+; GFX10:    s_waitcnt lgkmcnt(0)
+; GFX10:    s_setreg_b32 hwreg(HW_REG_MODE, 0, 3), s0
+; GFX10:    s_endpgm
+  call void @llvm.amdgcn.s.setreg(i32 2049, i32 3)
+  call void @llvm.amdgcn.s.setreg(i32 4097, i32 %var.mode)
+  call void asm sideeffect "", ""()
+  ret void
+}
+
+declare void @llvm.amdgcn.s.setreg(i32 immarg, i32)