[PATCH] D25975: AMDGPU/SI: Make f16 a legal type for VI subtargets

[Matt Arsenault via llvm-commits]

arsenm added a comment.

LGTM except for some nits



================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:2033-2035
+  if (ConstantFPSDNode *FP = dyn_cast<ConstantFPSDNode>(Op))
+    return DAG.getConstant(FP->getValueAPF().bitcastToAPInt().getZExtValue(),
+                           SDLoc(Op), MVT::i32);
----------------
Braces


================
Comment at: lib/Target/AMDGPU/SIShrinkInstructions.cpp:93-94
 
+      case AMDGPU::V_MAC_F16_e64:
       case AMDGPU::V_MAC_F32_e64:
         if (!isVGPR(Src2, TRI, MRI) ||
----------------
I would sort f16 after


================
Comment at: lib/Target/AMDGPU/VOP2Instructions.td:160-162
+def VOP_MAC_F16 : VOPProfile <[f16, f16, f16, f16]> {
+  let Ins32 = (ins Src0RC32:$src0, Src1RC32:$src1, VGPR_32:$src2);
+  let Ins64 = getIns64<Src0RC64, Src1RC64, RegisterOperand<VGPR_32>, 3,
----------------
I think it should be easy to make VOP_MAC be the class with the type operand to avoid copy pasting the entire thing just to change the type


================
Comment at: test/CodeGen/AMDGPU/fadd.f16.ll:1
+; RUN: llc -march=amdgcn -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI %s
+; RUN: llc -march=amdgcn -mcpu=fiji -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI %s
----------------
s/SI/CI


================
Comment at: test/CodeGen/AMDGPU/fcmp.f16.ll:1
+; RUN: llc -march=amdgcn -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=SI %s
+; RUN: llc -march=amdgcn -mcpu=fiji -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VI %s
----------------
Ditto


================
Comment at: test/CodeGen/AMDGPU/llvm.exp2.f16.ll:41
+; GCN: s_endpgm
+define void @vector_vt_exp(
+    <2 x half> addrspace(1)* %r,
----------------
The naming convention should not name this vector and end in the actual vector type, v2f16 so this can expand to other vector widths that we might need to test later


https://reviews.llvm.org/D25975