[PATCH] D128218: [AMDGPU] gfx11 VOPD instructions MC support

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added a comment.

Looks good, except for some nitpicking.



================
Comment at: llvm/lib/Target/AMDGPU/VOP2Instructions.td:144
+multiclass VOP2Inst_e32_VOPD<string opName,
+                        VOPProfile P,
+                        bits<5> VOPDOp,
----------------
Indent.


================
Comment at: llvm/lib/Target/AMDGPU/VOP2Instructions.td:290
 
+multiclass VOP2eInst_VOPD <string opName,
+                      VOPProfile P,
----------------
Is there a way to avoid code duplication here? `VOP2eInst` and `VOP2eInst_VOPD` are almost identical.


================
Comment at: llvm/lib/Target/AMDGPU/VOP2Instructions.td:291
+multiclass VOP2eInst_VOPD <string opName,
+                      VOPProfile P,
+                      bits<5> VOPDOp,
----------------
Indent.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:28
+  let Inst{48-41} = vsrc1Y;
+  let Inst{55-49} = vdstY{7 - 1};
+  let Inst{63-56} = vdstX;
----------------
Extra spaces in bit range.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:48
+  let Inst{48-41} = vsrc1Y;
+  let Inst{55-49} = vdstY{7 - 1};
+  let Inst{63-56} = vdstX;
----------------
Extra spaces in bit range.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:58
+class VOPD_Base<dag outs, dag ins, string asm, VOP_Pseudo VDX,
+           VOP_Pseudo VDY, VOPD_Component XasVC, VOPD_Component YasVC>
+    : VOPAnyCommon<outs, ins, asm, []>,
----------------
Indent.
What does `VC` stand for?


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:85
+class VOPD<dag outs, dag ins, string asm, VOP_Pseudo VDX, VOP_Pseudo VDY,
+                VOPD_Component XasVC, VOPD_Component YasVC>
+    : VOPD_Base<outs, ins, asm, VDX, VDY, XasVC, YasVC>,
----------------
Indent.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:87
+    : VOPD_Base<outs, ins, asm, VDX, VDY, XasVC, YasVC>,
+      VOPDe<XasVC.VOPDOp{3 - 0}, YasVC.VOPDOp> {
+  let Inst{16-9} = !if(!eq(VDX.Mnemonic,"v_mov_b32"),0x0,vsrc1X);
----------------
Extra spaces in bit range.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:95
+    : VOPD_Base<outs, ins, asm, VDX, VDY, XasVC, YasVC>,
+      VOPD_MADKe<XasVC.VOPDOp{3 - 0}, YasVC.VOPDOp> {
+  let Inst{16-9} = !if(!eq(VDX.Mnemonic,"v_mov_b32"),0x0,vsrc1X);
----------------
Extra spaces in bit range.


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:101
+
+// V_DUAL_DOT2ACC_F32_BF16  is a legal instruction, but V_DOT2ACC_F32_BF16 is not.
+// Since we generate the DUAL form by converting from the normal form we will
----------------
Extra space


================
Comment at: llvm/lib/Target/AMDGPU/VOPDInstructions.td:124
+    defvar YasVC = !cast<VOPD_Component>(y);
+    defvar isMADK = !or(!eq(x,"V_FMAAK_F32"),!eq(x,"V_FMAMK_F32"),!eq(y,"V_FMAAK_F32"),!eq(y,"V_FMAMK_F32"));
+    // If X or Y is MADK (have a mandatory immediate), all src operands which
----------------
Missing space after commas. Next 2 lines also need corrections.


================
Comment at: llvm/lib/Target/AMDGPU/VOPInstructions.td:35
+  Instruction BaseVOP = !cast<Instruction>(NAME);
+  string VOPDName = "v_dual_" # !substr(vOPDName,2);
+  bits<5> VOPDOp = OpIn;
----------------
Missing space after comma.


================
Comment at: llvm/lib/Target/AMDGPU/VOPInstructions.td:37
+  bits<5> VOPDOp = OpIn;
+  bit CanBeVOPDX = !le(VOPDOp, 13);
+}
----------------
Could we avoid hardcoding `13` here? Ideally it should be defined together with `VOPDXPseudos` and `VOPDYPseudos`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128218