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

[Changpeng Fang via Phabricator via llvm-commits]

cfang marked an inline comment as done.
cfang added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3508-3509
+// This is to lower INTRINSIC_W_CHAIN with illegal result types.
+SDValue SITargetLowering::lowerIntrinsicWChain(SDValue Op, SDValue &Chain,
+                                               SelectionDAG &DAG) const {
+  EVT LoadVT = Op.getValueType();
----------------
arsenm wrote:
> We already have LowerINTRINSIC_W_CHAIN, so this name could be confusing
This is actually the name you suggested before. 

Do you think lowerIntrinsicWChainWithIllegalReturnType is OK? Or any ither name suggestion? Thanks.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3564
+  Chain = Res.getValue(1);
+  if (Unpacked) { // From v2i32/v4i32 back to v2f16/v4f16.
+    // Truncate to v2i16/v4i16.
----------------
arsenm wrote:
> Move this into a helper function called in each switch rather than putting code after the switch. Not every intrinsic with an illegal type will necessarily be this situation
OK, will do as you suggested. Thanks.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3569
+    // Bitcast to original type (v2f16/v4f16).
+    return DAG.getNode(ISD::BITCAST, DL, LoadVT, Trunc);
+  } else // Cast back to the original packed type.
----------------
arsenm wrote:
> No return after else
What do you mean here "no return after else"? Do you mean there should not be a return statement, or a return statement is missing?
Thanks.


https://reviews.llvm.org/D38906