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

[Changpeng Fang via Phabricator via llvm-commits]

cfang marked 6 inline comments as done.
cfang added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3333
 
+static SDValue lowerIntrinsicWChain(SDValue Op, EVT EquivResultT,
+                                    SelectionDAG &DAG) {
----------------
arsenm wrote:
> 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
>SDValue Res = lowerIntrinsicWChain(Op, EquivLoadVT, DAG)

This is just a help function to replace the result type of "Op" with " EquivLoadVT".  maybe we should use a different name to avoid confusion.

What about: ReplaceResultType ? Thanks. 


================
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());
----------------
arsenm wrote:
> This is an unrelated change
Why do you think this is unrelated?  I have to handle Intrinsic::amdgcn_buffer_load_format in getTgtMemIntrinsic.
After return "true" there, I think it is better to make appropriate change here also.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4711
+      // TODO: Handle v3f16.
+      if (StoreVT == MVT::v2f16 || StoreVT== MVT::v4f16) {
+        if (!Subtarget->hasUnpackedD16VMem()) {
----------------
arsenm wrote:
> This is redundant with the other type check, you really want isVector() and to just assert on v3f16
Do you think we need to assert for v3f16? The code in this block has no reason not to handle v3f16. So the assert of v3f16, if any, should be somewhere else. Also MVY::v3f16 is not recognized in LC.


https://reviews.llvm.org/D38906