[PATCH] D155995: [AMDGPU]: Allow combining into v_dot4

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon added a comment.

Just some minor comments/questions.



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10749
+
+    // Only set IsSigned if the load is extended
+    if (L->getExtensionType() != ISD::NON_EXTLOAD)
----------------
add period at the end.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12410
+// Collect the ultimate src of each of the mul24 node's operands, and confirm
+// each operand is 8 bytes.
+static std::optional<ByteProvider<SDValue>>
----------------
This function is used for any mul, not just mul24?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12605
+    return IterIsSigned;
+  // If we have signed semantics with a MUL_U24 op, then fail
+  if (Src0.IsSigned.value_or(false) && MulOpcode == AMDGPUISD::MUL_U24)
----------------
I think the check includes any extend (or unknown?) as well as signed?

Maybe say, If we have MUL_u24 without unsigned semantics, then fail.



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12609
+  // If we have unsigned semantics with a MUL_I24 op, then fail
+  if (Src0.IsSigned && !*Src0.IsSigned && MulOpcode == AMDGPUISD::MUL_I24)
+    return IterIsSigned;
----------------
BTW, is this the same as :
if (!Src0.IsSigned.value_or(false) && MulOpcode == AMDGPUISD::MUL_I24)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155995