[PATCH] D150135: [RISCV] Improve RV64 codegen for i32 ISD::SADDO when RHS is constant.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9041
+
+    // If the RHS is a constant, we can simplify ConditionRHS below. Otherwise
+    // use the default legalization.
----------------
reames wrote:
> The uaddo case below uses a comparison of two add flavors.  Does the same notion work here for the non-constant case?
I don't think uaddo uses two add flavors. uaddo should be doing an ADDW and comparing the result to the sign extended version of the original LHS.

The default expansion for saddo does an i64 ADD with sign extended inputs and an ADDW. If the results from the ADD and ADDW don't match it overflowed. Is that were you suggesting?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9046
+
+    SDValue LHS = DAG.getNode(ISD::SIGN_EXTEND, DL, MVT::i64, N->getOperand(0));
+    SDValue RHS = DAG.getNode(ISD::SIGN_EXTEND, DL, MVT::i64, N->getOperand(1));
----------------
reames wrote:
> The uaddo/usubo case just below uses any_extend here, can we get away with the same?
LHS and RHS are used against by the compares which do need SIGN_EXTEND. The uaddo code generates a separate SIGN_EXTEND for LHS later. There must have been some reason I didn't sign extend before the ADD in uaddo but I don't remember why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150135