[PATCH] D58481: [AMDGPU] fix commuted case of sub combine

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 2 inline comments as done.
rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:8621
+  bool NeedCommute = Opc != ISD::SUBCARRY;
+  if (NeedCommute)
     std::swap(RHS, LHS);
----------------
arsenm wrote:
> This swap is hard to follow. It's not immediately clear to me why this would be commuted in != subcarry case. What happens in the sub (subcarry, subcarry) case?  I think it would be clearer to just explicitly handle the two cases
It really does not matter what is the RHS, it can be anything including subcarry.


================
Comment at: test/CodeGen/AMDGPU/combine-cond-add-sub.ll:151
+}
+
 ; GCN-LABEL: {{^}}sube_sub:
----------------
arsenm wrote:
> Maybe add a test where both operands to a sub are subcarry
That is really no different from any other instruction but complicates the test a lot since we cannot get subcarry right from the IR.


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

https://reviews.llvm.org/D58481