[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:3552-3553
+/// \brirf This is to lower INTRINSIC_W_CHAIN with illegal result types.
+SDValue SITargetLowering::lowerIntrinsicWChain(SDValue Op, SDValue *Chain,
+                                               SelectionDAG &DAG) const {
+  SDLoc SL(Op);
----------------
This is still structured strangely. Most of the code inside this function should be in a helper, and lowerIntrinsicWChain should have the switch over the intrinsic


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3565
+  if (SDValue Res = replaceResultType(Op, EquivLoadVT, DAG)) {
+    *Chain = Res.getOperand(0);
+    if (Unpacked) { // From v2i32/v4i32 back to v2f16/v4f16.
----------------
This is not the correct chain, this is the input chain, not the output result chain


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3572-3573
+      return DAG.getNode(ISD::BITCAST, SL, LoadVT, Trunc);
+    } else // Cast back to the original packed type.
+      return DAG.getNode(ISD::BITCAST, SL, LoadVT, Res);
+  }
----------------
No return after else


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3606
+  case ISD::INTRINSIC_W_CHAIN: {
+    SDValue Chain = SDValue();
+    if (SDValue Res = lowerIntrinsicWChain(SDValue(N, 0), &Chain, DAG)) {
----------------
Remove = SDValue()


================
Comment at: lib/Target/AMDGPU/SIISelLowering.h:63
 
+  /// \brirf This is to lower INTRINSIC_W_CHAIN with illegal result types.
+  SDValue lowerIntrinsicWChain(SDValue Op, SDValue *Chain,
----------------
Typo brirf, but autobrief is on so you can just remove it


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.td:1899
                    [!cast<string>(SIEncodingFamily.SDWA9)],
+                   [!cast<string>(SIEncodingFamily.GFX80)],
                    [!cast<string>(SIEncodingFamily.GFX9)]];
----------------
You should add a note explaining why this is here and that it should probably be removed at some point


https://reviews.llvm.org/D38906