[PATCH] D84420: [AMDGPU] Add v3f16/v3i16 support to SDag

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.
Herald added a subscriber: wenlei.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:961-962
+                          TLI.getTypeToTransformTo(*DAG.getContext(),
+                                                   Results[i].getValueType()) &&
+                      "Invalid type for widened vector");
+    if (IsWidened)
----------------
Flakebi wrote:
> arsenm wrote:
> > It looks like you intended this to be an assert?
> The check was indeed copied from the assert in SetWidenedVector, thanks for the notice.
> For context: We need to make this check finer so that SetWidenedVector does not get called for  return values which are not widened (like with tfe).
I'm still confused by this change. This is relying on getTypeToTransformTo returning other for other?


================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:841
   defm BUFFER_LOAD_FORMAT_D16_XYZ : MUBUF_Pseudo_Loads <
-    "buffer_load_format_d16_xyz", v3f16
+    "buffer_load_format_d16_xyz", v4f16
   >;
----------------
Can you leave this as v3f16 and swap out the register/pattern type inside MUBUF_Pseudo_Loads?


================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:1707
 
-multiclass MTBUF_LoadIntrinsicPat<SDPatternOperator name, ValueType vt,
-                                  string opcode> {
+multiclass MTBUF_LoadIntrinsicPatInner<SDPatternOperator name, ValueType vt,
+                                       string opcode, PatFrag st> {
----------------
Is there a better name to use here besides "Inner"?


================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:1774
   defm : MTBUF_LoadIntrinsicPat<SItbuffer_load_d16, v2f16, "TBUFFER_LOAD_FORMAT_D16_XY">;
+  defm : MTBUF_LoadIntrinsicPatInner<SItbuffer_load_d16, v4f16, "TBUFFER_LOAD_FORMAT_D16_XYZ",
+    mtbuf_intrinsic_load_v3f16<SItbuffer_load_d16>>;
----------------
Having this one use a different multiclass is weird looking. Why can't it directly use the same multiclass as the other cases?

I would expect this to look something like

class MTBUF_LoadIntrinsicPta<SDPatternOperator node, ValueType memvt, ValueType vt = memvt>
and then only override the vt in the weird v3 cases


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84420/new/

https://reviews.llvm.org/D84420