[PATCH] D126475: [AMDGPU] gfx11 vop3 instructions

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:816
+defm V_MAD_I64_I32_gfx11   : VOP3be_Real_gfx11<0x2ff, "V_MAD_I64_I32_gfx11", "v_mad_i64_i32">;
+// V_ADD_CO_U32, V_SUB_CO_U32, V_SUBREV_CO_U32 in VOP2Instructions.td
+defm V_CVT_PK_I16_F32      : VOP3_Realtriple_gfx11<0x306>;
----------------
dp wrote:
> What is so special with these opcodes that they are mentioned here? E.g. why v_add_f16 is not in this list?
v_add_f16 is significantly different, and will come in a later patch. There is nothing particularly special about those opcodes. I will remove the comment to aid in grepping.


================
Comment at: llvm/lib/Target/AMDGPU/VOPInstructions.td:661
 
-class VOP_DPP_Pseudo <string OpName, VOPProfile P, list<dag> pattern=[]> :
-  InstSI <P.OutsDPP, P.InsDPP, OpName#P.AsmDPP, pattern>,
+class VOP_DPP_Pseudo <string OpName, VOPProfile P, list<dag> pattern=[],
+  dag Ins = P.InsDPP, string asmOps = P.AsmDPP> :
----------------
dp wrote:
> This and the following DPP/DPP8 changes do not look relevant to this patch.
Moved to https://reviews.llvm.org/D126483


================
Comment at: llvm/lib/Target/AMDGPU/VOPInstructions.td:969
+}
+// In VOP1, we can have clamp and omod even if !HasModifiers
 class getVOP3Pat<VOPProfile P, SDPatternOperator node> {
----------------
dp wrote:
> This sounds rather cryptic to me. Why VOP1 only?
In vop2, HasModifiers = HasModifiers || HasClamp || HasOmod
In vop1, HasModifiers can be false if HasClamp or HasOmod is true. It is not ideal they are defined this way, but since this class aims to be a common interface for both, it must support both those cases. A later patch could refactor this to be more clear.


================
Comment at: llvm/test/MC/AMDGPU/gfx11_vop123.s:2244
+
+v_dot2_f16_f16 v0, v1, v2, v3 op_sel:[0,0,1,1]
+// GFX11: encoding: [0x00,0x60,0x66,0xd6,0x01,0x05,0x0e,0x04]
----------------
dp wrote:
> Out of 6400 tests I found only 4 tests with op_sel. Is there a reason for that? Are you planning to switch to sp3 syntax?
op_sel for vop3 is a work in progress. I expect more changes and tests to come at a later point in time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126475