[AMDGPU] gfx11 Fix VOP3 dot instructions

[Petar.Avramovic]

Summary

Matches sp3 behavior. op_sel[0:1] must be 0. abs and neg src modifiers for bf16

[Joe_Nash]

I agree with the problem description in the commit message, but I'm not sure this patch fully fixes the problem. There are no tests actually using the op_sel operand. I think those tests would reveal that the op_sel value is not being propagated from src_modifiers to the op_sel operand. We will need something similar to convertVOP3PDPPInst
In addition, OPSEL[1:0] are ignored by the hardware for these instructions. That should be reflected in the instruction definition. If it doesn't impact the dpp implementation, it could be a separate patch.

[dp]

This patch does enable assembly and disassembly of v_dot2_f16_f16_e64_dpp and v_dot2_bf16_bf16_e64_dpp (with op_sel=0). Currently, these instructions are simply rejected as if they are not defined.
I agree that this patch does not fix the op_sel issue, but that is another problem that may be addressed separately.

[Joe_Nash]

The problem with doing the work separately is that codegen will be affected by this patch as well. Currently, dot2_bf16_bf16_e64_dpp does not exist, so GCNDPPCombine will simply fail to generate that instruction. But after this patch we will generate it. Now this test I tried appears to code generate correctly, but the problem is we cannot assemble or disassemble the resulting instructions. So I would recommend the full asm/disasm implementation and including this codegen test too.

RUN: llc -march=amdgcn -mcpu=gfx1100 -verify-machineinstrs -o - %s | FileCheck %s -check-prefix=GCN

---

name: dot2_bf16_bf16
tracksRegLiveness: true
body: |

bb.0:
  liveins: $vgpr0, $vgpr1, $vgpr2

  %0:vgpr_32 = COPY $vgpr0
  %1:vgpr_32 = COPY $vgpr1
  %2:vgpr_32 = COPY $vgpr2
  %3:vgpr_32 = IMPLICIT_DEF
  %4:vgpr_32 = V_MOV_B32_dpp %3, %1, 1, 15, 15, 1, implicit $exec
  %5:vgpr_32 = V_DOT2_BF16_BF16_e64 8, %4, 4, %3, 4, %2, 0, implicit $exec
  S_ENDPGM 0, implicit %5

...

Yields
0xd6677000 0x040a00fa 0xff080101; v_dot2_bf16_bf16 v0.h, v1, v0, v2.h quad_perm:[1,0,0,0] bound_ctrl:1

[dp]

I see, thanks!

[Petar.Avramovic]

op_sel value is not being propagated from src_modifiers to the op_sel operand

I was convinced that op_sel value is not important for instruction printing. Non dpp version does not even have op_sel operand and still gets printed correctly, helper prints op_sel[a,b,c] from src_modifiers.
I missed to test assembler, missing part is setting op_sel bit in src_modifiers, done via cvtVOP3P.
I will double check both.

[Joe_Nash]

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8805 ↗

(On Diff #442382)

The conditional check here should really be checking for is DOT2_BF16_BF16_e64_dpp_gfx11 || DOT2_BF16_BF16_e64_dpp8_gfx11 || DOT2_F16_F16 ....
There is a similar conditional check in the Disassembler where we need to call convertVOP3PDPP.
Options: 1) check if Opc = the specific dot instructions 2) add a tablegen mapping helper table to check if something is one of the relevant instructions 3)
Since we are relying on checking the presence of operands in cvtVOP3P anyway, maybe we can unconditionally call cvtVOP3P here, and rename the function to cvtVOPModsHelper or something.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

763

This will set op_sel to the wrong value if it was input as 1. We need to call convertVOP3PDPPInst to copy the operand correctly, or something equivalent.
However, that defect does not appear to effect the output, because the op_sel operand does not set any bits in the output machine code.

llvm/lib/Target/AMDGPU/VOP3Instructions.td

705

DOT2_BF16_BF16_e64_dpp should disallow modifiers on src0 and src1, though DOT2_BF16_BF16_e64 allows them. They should allow op_sel on dst, but this may be an issue since the bit for that is in src0 modifiers.
DOT2_F16_F16_e64_dpp should allow abs and neg modifiers on all operands, but op_sel only on dst and src2.

Setting HasSrcMods effects codegen, and dpp16. Can you please include the codegen test I proposed or some variation on it? Can you also include asm and disasm tests for dpp16?

[Petar.Avramovic]

Matches sp3 behavior. op_sel[0:1] must be 0. abs and neg src modifiers for bf16
FixMe(wip, requires some td file changes) clang uses i16 for bf16, I used f16 to get abs and neg parsing support.

[dp]

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4199 ↗

(On Diff #445930)

Is it possible for an opcode to be both VOP3 and VOP3P?

llvm/lib/Target/AMDGPU/VOPInstructions.td

273–274 ↗

(On Diff #445930)

Bits op_sel[1:0] are ignored, so opcodes with these bits set to 1 are legal. Using ? instead of 0 would allow decoding of such opcodes.

1342 ↗

(On Diff #445930)

Looks like this is not necessary.

1380–1381 ↗

(On Diff #445930)

Ditto.

1403–1404 ↗

(On Diff #445930)

Ditto.

[Petar.Avramovic]

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4199 ↗

(On Diff #445930)

afaik no, I was surprised that VOP3P instructions also have VOP3 flag, VOP3 instructions don't have VOP3P flag (maybe a bug?). I could check opcodes (I think that there should be 6 opcodes).

llvm/lib/Target/AMDGPU/VOPInstructions.td

273–274 ↗

(On Diff #445930)

What is desired behavior for 'ignored bits' then?
assembler: report error if op_sel[1:0] are used (sp3 reports error) or parse the 1 and use 0 when printing/encoding instruction
disassembler: read 1 but encode as 0 anyway (sp3 does this) or fail to disassemble

[dp]

llvm/lib/Target/AMDGPU/VOPInstructions.td

273–274 ↗

(On Diff #445930)

I think that assembler should be strict and report an error if op_sel[1:0] bits are not 0. Disassembler should be able to decode instructions with ignored bits to aid in binary code analysis (ignored bits may be displayed as 0 in op_sel, this is fine).

[dp]

Could you add a disassembler test with ignored bits set to 1?

[Petar.Avramovic]

llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_vop3_dpp8.txt

5274 ↗

(On Diff #446130)

Like this? Ignored bits set to zero (0x18), check diff with previous in History tab there are a few more.

[dp]

Oh, sorry. I see that you have added them. Probably these tests need a comment to make clear what is being tested.

[Joe_Nash]

Overall it looks pretty good. The main thing I don't know is if making the bf16 operands float16 will work correctly with codegen. Hopefully @rampitec can answer.

llvm/include/llvm/IR/IntrinsicsAMDGPU.td

2072 ↗

(On Diff #446138)

@rampitec should look at these intrinsic changes. I am not familiar enough with other BF16 support to know if this is reasonable.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4199 ↗

(On Diff #445930)

Is it possible for an opcode to be both VOP3 and VOP3P?

It should not be, IMO. Those fields designate the encoding, and the instruction should only have one encoding. It could be considered a bug but a long standing one. I have been having similar discussions about an instruction being both VOPC and VOP3. @foad
That said, this check looks fine for the way things work now. We can come back and change it in a separate patch if desired, because I'm pretty sure there will be some other issues arising if that change is made.

llvm/lib/Target/AMDGPU/VOPInstructions.td

272 ↗

(On Diff #446138)

Make the parent class of this VOP3OpSel_gfx11. No functional change, but it seems better.

llvm/test/MC/AMDGPU/gfx11_asm_dpp16.s

1 ↗

(On Diff #446138)

Can you add --implicit-check-not=error to the runlines?

llvm/test/MC/AMDGPU/gfx11_asm_dpp8.s

1 ↗

(On Diff #446138)

Can you add --implicit-check-not=error to the runlines?

[Petar.Avramovic]

llvm/include/llvm/IR/IntrinsicsAMDGPU.td

2072 ↗

(On Diff #446138)

This only gets desired behavior in testing. The problem is that there is no bf16 in clang, i16 is used instead. Our td files rely on types. I am looking if can bypass type checks. It would be much cleaner if we had the bf16 type.

[rampitec]

In D129084#3665914, @Joe_Nash wrote:

Overall it looks pretty good. The main thing I don't know is if making the bf16 operands float16 will work correctly with codegen. Hopefully @rampitec can answer.

No, this is incorrect. bf16 is not the same as f16. One need a cast to/from integer type to use bf16.

[rampitec]

llvm/include/llvm/IR/IntrinsicsAMDGPU.td

2072 ↗

(On Diff #446138)

We cannot have bf16 type. This is not a question of compiler support, this is the support which does not exist in HW. If you can only do dots on the type you cannot generally support it.

[rampitec]

Please revert the part about using half instead of integers for bf16.

[Petar.Avramovic]

Fix modifiers for bf16

[Joe_Nash]

Can you add disassembler tests with op_sel:[1,1,1,1] and show the bit is ignored? Otherwise LGTM

[Joe_Nash]

llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_vop3_dpp8.txt

5287 ↗

(On Diff #446523)

Ok, I was confused because the AsmPrinter prints what the hardware will do with that instruction, not what the bits actually disassemble to. This matches sp3 and there are plenty of warnings how those bits are treated. I think this is good.

[rampitec]

LGTM with a nit.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

4198 ↗

(On Diff #446523)

Please add parenthesis around binary operations.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

 DecodeStatus AMDGPUDisassembler::convertDPP8Inst(MCInst &MI) const {
   unsigned Opc = MI.getOpcode();
   unsigned DescNumOps = MCII->get(Opc).getNumOperands();
   if (MCII->get(Opc).TSFlags & SIInstrFlags::VOP3P) {
     convertVOP3PDPPInst(MI);
   } else if ((MCII->get(Opc).TSFlags & SIInstrFlags::VOPC) ||
              AMDGPU::isVOPC64DPP(Opc)) {
     convertVOPCDPPInst(MI);
+  } else if(MCII->get(Opc).TSFlags & SIInstrFlags::VOP3) {
+    // Since op_sel is not last operand, we have to add it manually so that
+    // remaining operands, dpp8 and fi, are at correct operand index.
+    if (MI.getNumOperands() < DescNumOps &&
+        AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel) != -1)
+      insertNamedMCOperand(MI, MCOperand::createImm(0), AMDGPU::OpName::op_sel);
   } else {
     // Insert dummy unused src modifiers.
     if (MI.getNumOperands() < DescNumOps &&
         AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src0_modifiers) != -1)
       insertNamedMCOperand(MI, MCOperand::createImm(0),
                            AMDGPU::OpName::src0_modifiers);
 
     if (MI.getNumOperands() < DescNumOps &&

llvm/lib/Target/AMDGPU/VOP3Instructions.td

 }
 
 let WaveSizePredicate = isWave32 in {
 def : DivFmasPat<f32, V_DIV_FMAS_F32_e64, VCC_LO>;
 def : DivFmasPat<f64, V_DIV_FMAS_F64_e64, VCC_LO>;
 }
 
 class VOP3_DOT_Profile<VOPProfile P, VOP3Features Features = VOP3_REGULAR> : VOP3_Profile<P, Features> {
-  // FIXME VOP3 DPP versions are unsupported
-  let HasExtVOP3DPP = 0;
   let HasClamp = 0;
   let HasOMod = 0;
+  // Type calculations workaround for bf16.
+  let HasSrc0Mods = 1;
+  let HasSrc1Mods = 1;
+  let HasSrc2Mods = 1;
   let InsVOP3OpSel = getInsVOP3OpSel<Src0RC64, Src1RC64, Src2RC64,
                                 NumSrcArgs, HasClamp, HasOMod,
                                 !if(isFloatType<Src0VT>.ret, FPVRegInputMods, IntOpSelMods),
                                 !if(isFloatType<Src1VT>.ret, FPVRegInputMods, IntOpSelMods),
                                 !if(isFloatType<Src2VT>.ret, FPVRegInputMods, IntOpSelMods)>.ret;
 }
 
 let SubtargetPredicate = isGFX11Plus in {

 defm V_MAXMIN_F32          : VOP3_Realtriple_gfx11<0x25e>;
 defm V_MINMAX_F32          : VOP3_Realtriple_gfx11<0x25f>;
 defm V_MAXMIN_F16          : VOP3_Realtriple_gfx11<0x260>;
 defm V_MINMAX_F16          : VOP3_Realtriple_gfx11<0x261>;
 defm V_MAXMIN_U32          : VOP3_Realtriple_gfx11<0x262>;
 defm V_MINMAX_U32          : VOP3_Realtriple_gfx11<0x263>;
 defm V_MAXMIN_I32          : VOP3_Realtriple_gfx11<0x264>;
 defm V_MINMAX_I32          : VOP3_Realtriple_gfx11<0x265>;
-// FIXME VOP3 DPP Dot instructions are unsupported
-defm V_DOT2_F16_F16        : VOP3_Real_Base_gfx11<0x266>;
-defm V_DOT2_BF16_BF16      : VOP3_Real_Base_gfx11<0x267>;
+defm V_DOT2_F16_F16        : VOP3_Realtriple_gfx11<0x266>;
+defm V_DOT2_BF16_BF16      : VOP3_Realtriple_gfx11<0x267>;
 defm V_DIV_SCALE_F32       : VOP3be_Real_gfx11<0x2fc, "V_DIV_SCALE_F32", "v_div_scale_f32">;
 defm V_DIV_SCALE_F64       : VOP3be_Real_gfx11<0x2fd, "V_DIV_SCALE_F64", "v_div_scale_f64">;
 defm V_MAD_U64_U32_gfx11   : VOP3be_Real_gfx11<0x2fe, "V_MAD_U64_U32_gfx11", "v_mad_u64_u32">;
 defm V_MAD_I64_I32_gfx11   : VOP3be_Real_gfx11<0x2ff, "V_MAD_I64_I32_gfx11", "v_mad_i64_i32">;
 defm V_ADD_NC_U16          : VOP3Only_Realtriple_gfx11<0x303>;
 defm V_SUB_NC_U16          : VOP3Only_Realtriple_gfx11<0x304>;
 defm V_MUL_LO_U16          : VOP3Only_Realtriple_gfx11<0x305>;
 defm V_CVT_PK_I16_F32      : VOP3_Realtriple_gfx11<0x306>;

llvm/test/MC/AMDGPU/gfx11_asm_dpp.s

 // GFX11: encoding: [0x00,0x01,0x5f,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0xc6,0xfa]
 
 v_fma_f32_e64_dpp v80, v81, v82, v81 dpp8:[0,1,6,3,4,5,6,7]
 // GFX11: encoding: [0x50,0x00,0x13,0xd6,0xe9,0xa4,0x46,0x05,0x51,0x88,0xc7,0xfa]
 
 v_fma_f32_e64_dpp v80, v81, abs(v82), v81 dpp8:[0,1,6,3,4,5,6,7]
 // GFX11: encoding: [0x50,0x02,0x13,0xd6,0xe9,0xa4,0x46,0x05,0x51,0x88,0xc7,0xfa]
 
+v_dot2_f16_f16_e64_dpp v0, v1, v2, v3 dpp8:[0,1,2,3,4,4,4,4]
+// GFX11: encoding: [0x00,0x00,0x66,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92]
+
+v_dot2_bf16_bf16_e64_dpp v0, v1, v2, v3 dpp8:[0,1,2,3,4,4,4,4]
+// GFX11: encoding: [0x00,0x00,0x67,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92]
+
 v_max3_f32_e64_dpp v5, v1, v2, s3 dpp8:[7,6,5,4,3,2,1,0]
 // GFX11: encoding: [0x05,0x00,0x1c,0xd6,0xe9,0x04,0x0e,0x00,0x01,0x77,0x39,0x05]
 
 v_add3_u32_e64_dpp v5, v1, v2, -1 dpp8:[7,6,5,4,3,2,1,0]
 // GFX11: encoding: [0x05,0x00,0x55,0xd6,0xe9,0x04,0x06,0x03,0x01,0x77,0x39,0x05]
 
 v_add3_u32_e64_dpp v5, v1, v2, s4 dpp8:[7,6,5,4,3,2,1,0]
 // GFX11: encoding: [0x05,0x00,0x55,0xd6,0xe9,0x04,0x12,0x00,0x01,0x77,0x39,0x05]

llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_all.txt

 0x00,0x20,0x20,0xcc,0x01,0x05,0x0e,0x04
 
 # GFX11: v_fma_mixhi_f16 v0, v1, v2, v3 op_sel_hi:[1,1,1] clamp ; encoding: [0x00,0xc0,0x22,0xcc,0x01,0x05,0x0e,0x1c]
 0x00,0xc0,0x22,0xcc,0x01,0x05,0x0e,0x1c
 
 # GFX11: v_fma_mixlo_f16 v0, |v1|, -v2, |v3|     ; encoding: [0x00,0x05,0x21,0xcc,0x01,0x05,0x0e,0x44]
 0x00,0x05,0x21,0xcc,0x01,0x05,0x0e,0x44
 
+# GFX11: v_dot2_f16_f16_e64_dpp v0, v1, v2, v3 dpp8:[0,1,2,3,4,4,4,4] ; encoding: [0x00,0x00,0x66,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92]
+0x00,0x00,0x66,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92
+
+# GFX11: v_dot2_bf16_bf16_e64_dpp v0, v1, v2, v3 dpp8:[0,1,2,3,4,4,4,4] ; encoding: [0x00,0x00,0x67,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92]
+0x00,0x00,0x67,0xd6,0xe9,0x04,0x0e,0x04,0x01,0x88,0x46,0x92
+
 # GFX11: v_dot2_f32_f16_e64_dpp v0, v1, v2, v3 neg_lo:[0,1,1] neg_hi:[1,0,1] dpp8:[7,6,5,4,3,2,1,0] ; encoding: [0x00,0x05,0x13,0xcc,0xe9,0x04,0x0e,0xc4,0x01,0x77,0x39,0x05]
 0x00,0x05,0x13,0xcc,0xe9,0x04,0x0e,0xc4,0x01,0x77,0x39,0x05
 
 # GFX11: v_dot2_f32_f16_e64_dpp v0, v1, v2, v3 neg_lo:[1,1,0] neg_hi:[1,0,1] quad_perm:[3,2,1,0] row_mask:0xf bank_mask:0xe ; encoding: [0x00,0x05,0x13,0xcc,0xfa,0x04,0x0e,0x64,0x01,0x1b,0x00,0xfe]
 0x00,0x05,0x13,0xcc,0xfa,0x04,0x0e,0x64,0x01,0x1b,0x00,0xfe
 
 # GFX11: v_dot2_f32_f16_e64_dpp v0, v1, v2, v3 quad_perm:[2,2,3,1] row_mask:0xf bank_mask:0xf bound_ctrl:1 fi:1 ; encoding: [0x00,0x00,0x13,0xcc,0xfa,0x04,0x0e,0x04,0x01,0x7a,0x0c,0xff]
 0x00,0x00,0x13,0xcc,0xfa,0x04,0x0e,0x04,0x01,0x7a,0x0c,0xff