[PATCH] D126989: [AMDGPU] gfx11 VOPC instructions

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp:628
 
+bool AMDGPUInstPrinter::needsVccMods(const MCInstrDesc &Desc,
+                                     unsigned OpNo) const {
----------------
The name of the method is somewhat unexpected - what does "Mods" mean? Maybe "needsImpliedVcc" would be better?


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:63
+  let InsDPP = getInsDPP<VOPDstOperand<Src0DPP>, Src0DPP, Src1DPP, Src2DPP, NumSrcArgs,
+                                   HasModifiers, Src0ModDPP, Src1ModDPP, Src2ModDPP>.ret;
+  let InsDPP16 = getInsDPP16<VOPDstOperand<Src0DPP>, Src0DPP, Src1DPP, Src2DPP, NumSrcArgs,
----------------
Indent


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:65
+  let InsDPP16 = getInsDPP16<VOPDstOperand<Src0DPP>, Src0DPP, Src1DPP, Src2DPP, NumSrcArgs,
+                                   HasModifiers, Src0ModDPP, Src1ModDPP, Src2ModDPP>.ret;
+  let InsDPP8 = getInsDPP8<VOPDstOperand<Src0DPP>, Src0DPP, Src1DPP, Src2DPP,
----------------
Ditto


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:67-68
+  let InsDPP8 = getInsDPP8<VOPDstOperand<Src0DPP>, Src0DPP, Src1DPP, Src2DPP,
+                                 NumSrcArgs, HasModifiers,
+                                 Src0ModDPP, Src1ModDPP, Src2ModDPP>.ret;
+
----------------
Ditto


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1032
+
+  let Inst{8 - 0} = 0xfa;
+
----------------
Most of the code has no spaces around '-' when specifying bit ranges, but the code here and below is different. We should follow the style of existing code. https://llvm.org/docs/CodingStandards.html#golden-rule.


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1074
+  let OtherPredicates = ps.OtherPredicates;
+  let Constraints          = ps.Constraints;
+  let AsmMatchConverter = "cvtVOPCNoDstDPP";
----------------
Extra spaces.


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1130
+class VOPC64_DPP16_Dst<bits<10> op, VOP_DPP_Pseudo ps,
+                         string opName = ps.OpName>
+    : VOPC64_DPP16<op, ps, opName> {
----------------
Indent


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1137
+class VOPC64_DPP16_NoDst<bits<10> op, VOP_DPP_Pseudo ps,
+                           string opName = ps.OpName>
+    : VOPC64_DPP16<op, ps, opName> {
----------------
Ditto


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1199-1202
+        // Encoding used for VOPC instructions encoded as VOP3 differs from
+        // VOP3e by destination name (sdst) as VOPC doesn't have vector dst.
+        bits<8> sdst;
+        let Inst{7-0} = sdst;
----------------
Indent is correct but should be increased for better readability (or ':' may be moved to the next line).


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1214
+            : VOPC_DPP16_SIMC<op{7-0}, psDPP,
+                         SIEncodingFamily.GFX11>;
+        def _e32_dpp_w32_gfx11
----------------
Indent


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1413-1414
+            : VOPC_DPP16_SIMC<op{7-0},
+                         psDPP,
+                         SIEncodingFamily.GFX11> {
+          let AsmString = !subst("_nosdst", "", psDPP.OpName) # " " # AsmDPP;
----------------
Indent


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1498-1499
+    }
+
+
+  }
----------------
Extra line


================
Comment at: llvm/lib/Target/AMDGPU/VOPCInstructions.td:1685
+
+
 //===----------------------------------------------------------------------===//
----------------
Extra line


================
Comment at: llvm/test/MC/AMDGPU/gfx11_asm_vopc_dpp.s:118
+
+
+; ================================================================
----------------
Extra line


================
Comment at: llvm/test/MC/AMDGPU/gfx11_asm_vopc_dpp.s:121
+; dpp
+; ================================================================
+
----------------
IMO the tests would look cleaner with uniform comments token ('//') and uniform (3-line) separator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126989