[PATCH] D124976: [AArch64] Fix sub with carry

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3321
 
-// Value is 1 if 'C' bit of NZCV is 1, else 0
-static SDValue carryFlagToValue(SDValue Flag, EVT VT, SelectionDAG &DAG) {
+// Value is 1 if 'C' bit of NZCV is 1, else 0 if !Invert.
+static SDValue carryFlagToValue(SDValue Flag, EVT VT, SelectionDAG &DAG,
----------------
The comment here could be improved.  Maybe spell out "if Invert is true, value is 0 if 'C' bit is set, and 1 if it is not set".


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3366
+      IsSigned ? overflowFlagToValue(Sum.getValue(1), VT1, DAG)
+               : carryFlagToValue(Sum.getValue(1), VT1, DAG, InvertCarry);
 
----------------
It's a bit concerning to me that you're changing OutFlag, but not OpCarryIn.  If we're inverting the output flag, don't we need to invert the input flag too?  Or is it already getting inverted correctly?

It might make sense to also add tests for i256 addition and subtraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124976