[PATCH] D54882: [AMDGPU] Add sdwa support for ADD|SUB U64 decomposed Pseudos

[Ron Lieberman via Phabricator via llvm-commits]

ronlieb marked 10 inline comments as done.
ronlieb added a comment.

New patch arriving momentarily ...



================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:922
+  // Make the new instruction.
+  auto NewInst =
+    BuildMI(*MISucc.getParent(), MISucc, MISucc.getDebugLoc(), SDWADesc);
----------------
rampitec wrote:
> You need to check for modifiers which you are going to drop by this conversion. For instance incoming instruction can be VOP3 OpSel. If it is OpSel you need to check that OpSel modifiers are trivial (e.g. have no effect and equivalent to VOP2). The same about VOP3 modifiers abs and neg.
> 
> You also need a mir test to check all negative cases when you cannot fold.
leaving this one open, to work on the MIR test, and think about what modifiers might affect the V_ADD and B_SUB instructions.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:961-962
 
+  } else if (MI.getOpcode() == AMDGPU::V_ADD_I32_e64 ||
+             MI.getOpcode() == AMDGPU::V_SUB_I32_e64) {
+    if (!opConvertedToVOP2(MI, ST))
----------------
rampitec wrote:
> arsenm wrote:
> > You have code checking for the carry ins, but you don't handle those here
> Right. It does not seem to be specific to just these instructions. In general any VOP3 can go through it.
The CarryIn/Out related code is located in pseudoOpConvertedToVOP2()


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:961-962
 
+  } else if (MI.getOpcode() == AMDGPU::V_ADD_I32_e64 ||
+             MI.getOpcode() == AMDGPU::V_SUB_I32_e64) {
+    if (!opConvertedToVOP2(MI, ST))
----------------
ronlieb wrote:
> rampitec wrote:
> > arsenm wrote:
> > > You have code checking for the carry ins, but you don't handle those here
> > Right. It does not seem to be specific to just these instructions. In general any VOP3 can go through it.
> The CarryIn/Out related code is located in pseudoOpConvertedToVOP2()
i changed the name to pseudoOpConvertedToVop2 to reflect that
we look for a possible ADD or SUB that resulted from a previously lowered 
V_ADD_U64_PSEUDO or V_SUB_U64_PSEUDO. The function pseudoOpConvertedToVOP2  further validates that we have a lowered pseudo and returns true if it was able to perform the conversion.

also added comments kind of like the above ...



Repository:
  rL LLVM

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

https://reviews.llvm.org/D54882