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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:8621
+  bool NeedCommute = Opc != ISD::SUBCARRY;
+  if (NeedCommute)
     std::swap(RHS, LHS);
----------------
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


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:8625
   if (LHS.getOpcode() == ISD::SUBCARRY) {
     // sub (subcarry x, 0, cc), y => subcarry x, y, cc
     auto C = dyn_cast<ConstantSDNode>(LHS.getOperand(1));
----------------
This needs a comment with the commuted case


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:8626-8627
     // sub (subcarry x, 0, cc), y => subcarry x, y, cc
     auto C = dyn_cast<ConstantSDNode>(LHS.getOperand(1));
     if (!C || C->getZExtValue() != 0)
       return SDValue();
----------------
This can use C->isNullValue() instead of getZExtValue


================
Comment at: test/CodeGen/AMDGPU/combine-cond-add-sub.ll:151
+}
+
 ; GCN-LABEL: {{^}}sube_sub:
----------------
Maybe add a test where both operands to a sub are subcarry


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

https://reviews.llvm.org/D58481