AMDGPU/GlobalISel: Implement select() for @llvm.amdgcn.cvt.pkrtz

[arsenm]

Have you tried enabling the tablegen selector build? I’ve been avoiding commiting any of these kind of simple selectors without that

[tstellar]

In D45907#1074205, @arsenm wrote:

Have you tried enabling the tablegen selector build? I’ve been avoiding commiting any of these kind of simple selectors without that

Yes, it only generates code for stand-alone patterns and instruction patterns that use specify register classes for operands rather than types.

[tstellar]

Actually, I just discovered -warn-on-skipped-patterns on it looks like most patterns are skipped because the ComplexPatterns is not mapped to GlobalISel. It looks like the pkrtz pattern doesn't match because it uses a SDNode.

[arsenm]

Have you tried enabling the tablegen selector build? I’ve been avoiding commiting any of these kind of simple selectors without that

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

165

Separate line

464

Ditto

test/CodeGen/AMDGPU/GlobalISel/inst-select-amdgcn.cvt.pkrtz.mir

5–9

I think you can drop the IR section, even with the intrinsic declaration

[tstellar]

Rewrite patch to use TableGen'd instruction selector.

test/CodeGen/AMDGPU/GlobalISel/inst-select-amdgcn.cvt.pkrtz.mir

5–9

It complained about function cvt_pkrtz not being defined in the IR, but I was able to drop the intrinsic declaration.

[tstellar]

Rebase to pull in updated TableGen pattern classes.

[tstellar]

Ping.

[arsenm]

LGTM

lib/Target/AMDGPU/AMDGPUGISel.td

class GISelVop2Pat <
   Instruction inst,
   ValueType dst_vt, RegisterClass rc0, RegisterClass rc1 = rc0,
   ValueType src0_vt = dst_vt, ValueType src1_vt = src0_vt>   : GCNPat <
 
   (dst_vt (node (src0_vt rc0:$src0), (src1_vt rc1:$src1))),
   (inst src0_vt:$src0, src1_vt:$src1)
 >;
 
+class GISelVop2CommutePat <
+  SDPatternOperator node,
+  Instruction inst,
+  ValueType dst_vt, RegisterClass rc0, RegisterClass rc1 = rc0,
+  ValueType src0_vt = dst_vt, ValueType src1_vt = src0_vt>   : GCNPat <
+
+  (dst_vt (node (src0_vt rc0:$src0), (src1_vt rc1:$src1))),
+  (inst src1_vt:$src1, src0_vt:$src0)
+>;
+
 multiclass GISelVOP2DefaultPat <
-  SDPatternOperator node, Instruction inst, ValueType vt> {
+  SDPatternOperator node, Instruction inst,
+  ValueType dst_vt, ValueType src_vt = dst_vt> {
+
+  def : GISelVop2Pat <node, inst, dst_vt, VGPR_32, VGPR_32, src_vt>;
 
-  def : GISelVop2Pat <node, inst, vt, VGPR_32>;
+  def : GISelVop2Pat <node, inst, dst_vt, SReg_32, VGPR_32, src_vt>;
 
-  def : GISelVop2Pat <node, inst, vt, SReg_32, VGPR_32>;
+  // FIXME: Intrinsics aren't marked as commutable, so we need to add an explcit
+  // pattern to handle commuting.  This is another reason why legalizing to a
+  // generic machine instruction may be better that matching the intrinsic
+  // directly.
+  def : GISelVop2CommutePat <node, inst, dst_vt, VGPR_32, SReg_32, src_vt>;
 }
 
 defm : GISelVOP2DefaultPat <or, V_OR_B32_e64, i32>;
 
+// FIXME: Select directly to _e32 so we don't need to deal with modifiers.
+// FIXME: We can't re-use SelectionDAG patterns here because they match
+// against a custom SDNode and we would need to create a generic machine
+// instruction that is equivalent to the custom SDNode.  This would also require
+// us to custom legalize the intrinsic to the new generic machine instruction,
+// but I can't get custom legalizing of intrinsic to work and I'm not sure if
+// this is even supported yet.
+defm : GISelVOP2DefaultPat <
+  int_amdgcn_cvt_pkrtz, V_CVT_PKRTZ_F16_F32_e32, v2f16, f32>;
+
+
 def gi_vop3mods0 :
     GIComplexOperandMatcher<s32, "selectVOP3Mods0">,
     GIComplexPatternEquiv<VOP3Mods0>;

lib/Target/AMDGPU/AMDGPUInstructionSelector.h

private:
   /// tblgen-erated 'select' implementation.
   bool selectImpl(MachineInstr &I, CodeGenCoverage &CoverageInfo) const;
 
   MachineOperand getSubOperand64(MachineOperand &MO, unsigned SubIdx) const;
   bool selectCOPY(MachineInstr &I) const;
   bool selectG_CONSTANT(MachineInstr &I) const;
   bool selectG_ADD(MachineInstr &I) const;
   bool selectG_GEP(MachineInstr &I) const;
+  bool selectG_INTRINSIC(MachineInstr &I, CodeGenCoverage &CoverageInfo) const;
   bool hasVgprParts(ArrayRef<GEPInfo> AddrInfo) const;
   void getAddrModeInfo(const MachineInstr &Load, const MachineRegisterInfo &MRI,
                        SmallVectorImpl<GEPInfo> &AddrInfo) const;
   bool selectSMRD(MachineInstr &I, ArrayRef<GEPInfo> AddrInfo) const;
   bool selectG_LOAD(MachineInstr &I) const;
   bool selectG_STORE(MachineInstr &I) const;
 
   InstructionSelector::ComplexRendererFns

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

bool AMDGPUInstructionSelector::selectG_ADD(MachineInstr &I) const {
   I.eraseFromParent();
   return true;
 }
 
 bool AMDGPUInstructionSelector::selectG_GEP(MachineInstr &I) const {
   return selectG_ADD(I);
 }
 
+bool AMDGPUInstructionSelector::selectG_INTRINSIC(MachineInstr &I,
+                                          CodeGenCoverage &CoverageInfo) const {
+  unsigned IntrinsicID =  I.getOperand(1).getIntrinsicID();
+
+  switch (IntrinsicID) {
+  default:
+    break;
+  case Intrinsic::amdgcn_cvt_pkrtz:
+    return selectImpl(I, CoverageInfo);
+  }
+  return false;
+}
+
 bool AMDGPUInstructionSelector::selectG_STORE(MachineInstr &I) const {
   MachineBasicBlock *BB = I.getParent();
   DebugLoc DL = I.getDebugLoc();
 
   // FIXME: Select store instruction based on address space
   MachineInstr *Flat = BuildMI(*BB, &I, DL, TII.get(AMDGPU::FLAT_STORE_DWORD))
           .add(I.getOperand(1))
           .add(I.getOperand(0))
bool AMDGPUInstructionSelector::selectG_LOAD(MachineInstr &I) const {
 
   bool Ret = constrainSelectedInstRegOperands(*Flat, TII, TRI, RBI);
   I.eraseFromParent();
   return Ret;
 }
 
 bool AMDGPUInstructionSelector::select(MachineInstr &I,
                                        CodeGenCoverage &CoverageInfo) const {
 
   if (!isPreISelGenericOpcode(I.getOpcode()))
     return true;
 
   switch (I.getOpcode()) {
   default:
     break;
   case TargetOpcode::G_FPTOUI:
   case TargetOpcode::G_OR:
     return selectImpl(I, CoverageInfo);
   case TargetOpcode::G_ADD:
     return selectG_ADD(I);
   case TargetOpcode::G_BITCAST:
     return selectCOPY(I);
   case TargetOpcode::G_CONSTANT:
     return selectG_CONSTANT(I);
   case TargetOpcode::G_GEP:
     return selectG_GEP(I);
+  case TargetOpcode::G_INTRINSIC:
+    return selectG_INTRINSIC(I, CoverageInfo);
   case TargetOpcode::G_LOAD:
     return selectG_LOAD(I);
   case TargetOpcode::G_STORE:
     return selectG_STORE(I);
   }
   return false;
 }
 

test/CodeGen/AMDGPU/GlobalISel/inst-select-amdgcn.cvt.pkrtz.mir

+# RUN: llc -march=amdgcn -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s -check-prefixes=GCN
+
+--- |
+  define void @cvt_pkrtz(i32 addrspace(1)* %global0) { ret void }
+...
+---
+
+name:            cvt_pkrtz
+legalized:       true
+regBankSelected: true
+
+# GCN-LABEL: name: cvt_pkrtz
+body: |
+  bb.0:
+    liveins: $sgpr0, $vgpr0, $vgpr1, $vgpr3_vgpr4
+
+    ; GCN: [[SGPR0:%[0-9]+]]:sreg_32_xm0 = COPY $sgpr0
+    %0:sgpr(s32) = COPY $sgpr0
+    ; GCN: [[VGPR0:%[0-9]+]]:vgpr_32 = COPY $vgpr0
+    %1:vgpr(s32) = COPY $vgpr0
+    ; GCN: [[VGPR1:%[0-9]+]]:vgpr_32 = COPY $vgpr1
+    %2:vgpr(s32) = COPY $vgpr1
+    %3:vgpr(s64) = COPY $vgpr3_vgpr4
+
+    ; cvt_pkrtz vs
+    ; GCN: V_CVT_PKRTZ_F16_F32_e32 [[SGPR0]], [[VGPR0]]
+    %4:vgpr(<2 x s16>) = G_INTRINSIC intrinsic(@llvm.amdgcn.cvt.pkrtz), %1, %0
+
+    ; cvt_pkrtz sv
+    ; GCN: V_CVT_PKRTZ_F16_F32_e32 [[SGPR0]], [[VGPR0]]
+    %5:vgpr(<2 x s16>) = G_INTRINSIC intrinsic(@llvm.amdgcn.cvt.pkrtz), %0, %1
+
+    ; cvt_pkrtz vv
+    ; GCN: V_CVT_PKRTZ_F16_F32_e32 [[VGPR0]], [[VGPR1]]
+    %6:vgpr(<2 x s16>) = G_INTRINSIC intrinsic(@llvm.amdgcn.cvt.pkrtz), %1, %2
+
+    %7:vgpr(s32) = G_BITCAST %4
+    %8:vgpr(s32) = G_BITCAST %5
+    %9:vgpr(s32) = G_BITCAST %6
+    G_STORE %7, %3 :: (store 4 into %ir.global0)
+    G_STORE %8, %3 :: (store 4 into %ir.global0)
+    G_STORE %9, %3 :: (store 4 into %ir.global0)
+...
+---