[PATCH] D38906: AMDGPU/SI: Implement d16 support for buffer intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:556-561
+static bool isHalfVT (EVT VT) {
+  return (VT == MVT::f16   ||
+          VT == MVT::v2f16 ||
+          //VT == MVT::v3f16 ||
+          VT == MVT::v4f16);
+}
----------------
You don't need this, you can just do VT.getScalarType()


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3333
 
+static SDValue lowerIntrinsicWChain(SDValue Op, EVT EquivResultT,
+                                    SelectionDAG &DAG) {
----------------
I'm confused because all of the intrinsic logic isn't contained here, and this also just always uses the d16 version which is wrong


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3412
+
+    bool HasPacked = !Subtarget->hasUnpackedD16VMem();
+    EVT UnpackedLoadVT = (LoadVT == MVT::v2f16) ? MVT::v2i32 : MVT::v4i32;
----------------
The only use of this is the negated condition, so just don't negate it


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4390-4393
     unsigned Opc = (IntrID == Intrinsic::amdgcn_buffer_load) ?
-        AMDGPUISD::BUFFER_LOAD : AMDGPUISD::BUFFER_LOAD_FORMAT;
-    EVT VT = Op.getValueType();
-    EVT IntVT = VT.changeTypeToInteger();
-
-    MachineMemOperand *MMO = MF.getMachineMemOperand(
-      MachinePointerInfo(MFI->getBufferPSV()),
-      MachineMemOperand::MOLoad,
-      VT.getStoreSize(), VT.getStoreSize());
-
-    return DAG.getMemIntrinsicNode(Opc, DL, Op->getVTList(), Ops, IntVT, MMO);
+                    AMDGPUISD::BUFFER_LOAD : AMDGPUISD::BUFFER_LOAD_FORMAT;
+    return DAG.getMemIntrinsicNode(Opc, DL, Op->getVTList(), Ops,
+                                   M->getMemoryVT(), M->getMemOperand());
----------------
This is an unrelated change


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4711
+      // TODO: Handle v3f16.
+      if (StoreVT == MVT::v2f16 || StoreVT== MVT::v4f16) {
+        if (!Subtarget->hasUnpackedD16VMem()) {
----------------
This is redundant with the other type check, you really want isVector() and to just assert on v3f16


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4720
+          }
+        } else {// We need to unpack the packed data to store.
+          EVT IntStoreVT = StoreVT.changeTypeToInteger();
----------------
Missing space before //


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.buffer.store.format.d16.ll:1
+; RUN: llc < %s -march=amdgcn -mcpu=tonga -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=UNPACKED %s
+; RUN: llc < %s -march=amdgcn -mcpu=gfx810 -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=PACKED -check-prefix=GFX81 %s
----------------
Use -enable-var-scope


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.tbuffer.load.d16.ll:1
+; RUN: llc < %s -march=amdgcn -mcpu=tonga -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=UNPACKED %s
+; RUN: llc < %s -march=amdgcn -mcpu=gfx810 -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=PACKED %s
----------------
-enable-var-scope


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.tbuffer.store.d16.ll:1
+; RUN: llc < %s -march=amdgcn -mcpu=tonga -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=UNPACKED %s
+; RUN: llc < %s -march=amdgcn -mcpu=gfx810 -verify-machineinstrs | FileCheck -check-prefix=GCN -check-prefix=PACKED -check-prefix=GFX81 %s
----------------
-enable-var-scope


https://reviews.llvm.org/D38906