[PATCH] D129084: [AMDGPU] gfx11 Fix VOP3 dot instructions

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added a comment.

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.



================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:2072
   Intrinsic<
-    [llvm_i16_ty],   // %r
+    [llvm_half_ty],  // %r
     [
----------------
@rampitec should look at these intrinsic changes. I am not familiar enough with other BF16 support to know if this is reasonable. 


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4199
+  if (TSFlags & SIInstrFlags::IsDOT && TSFlags & SIInstrFlags::VOP3 &&
+      !(TSFlags & SIInstrFlags::VOP3P)) {
+    int OpSelIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel);
----------------
Petar.Avramovic wrote:
> dp wrote:
> > Is it possible for an opcode to be both `VOP3` and `VOP3P`?
> 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).
> 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.



================
Comment at: llvm/lib/Target/AMDGPU/VOPInstructions.td:272
 
+class VOP3DotOpSel_gfx11<bits<10> op, VOPProfile p> : VOP3OpSel_gfx10<op, p>{
+  let Inst{11} = ?;
----------------
Make the parent class of this VOP3OpSel_gfx11. No functional change, but it seems better.


================
Comment at: llvm/test/MC/AMDGPU/gfx11_asm_dpp16.s:1
-// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefixes=GFX11 %s
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefixes=GFX11 %s
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s 2>&1 | FileCheck --check-prefixes=GFX11-ERR %s
----------------
Can you add --implicit-check-not=error to the runlines?


================
Comment at: llvm/test/MC/AMDGPU/gfx11_asm_dpp8.s:1
-// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefixes=GFX11 %s
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefixes=GFX11 %s
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s 2>&1 | FileCheck --check-prefixes=GFX11-ERR %s
----------------
Can you add --implicit-check-not=error to the runlines?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129084/new/

https://reviews.llvm.org/D129084