[PATCH] D29872: Do not legalize large add with addc/adde, introduce addcarry and do it with uaddo/addcarry
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 3 08:20:48 PDT 2017
spatel added a comment.
I haven't looked at carry codegen enough to approve, but see inline comments for some nits.
================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:215-218
/// Carry-setting nodes for multiple precision addition and subtraction.
/// These nodes take two operands of the same value type, and produce two
/// results. The first result is the normal add or sub result, the second
/// result is the carry flag result.
----------------
If I'd just stumbled into this code, I'd be confused why we have ADDC and ADDE and ADDCARRY. If the intent is to remove these, please add a 'FIXME' comment here that says we should get rid of these.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2084-2086
+ // canonicalize constant to RHS
+ ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0);
+ ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1);
----------------
This is copied from above, but comments are supposed to be full sentences (capital letter, period).
Use 'auto *' with dyn_cast because the type is obvious from the code and variable name.
================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:517
+ SDValue Ops[3] = { N->getOperand(0), N->getOperand(1) };
+ unsigned C = N->getNumOperands();
+ assert(C <= 3 && "Too many operands");
----------------
I'd call this 'NumOps' or similar. 'C' in LLVM usually means some kind of constant value.
================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:913-914
+ // ADDCARRY and SUBCARRY are new additions which needs
+ // to default to expand to not break all backends.
+ setOperationAction(ISD::ADDCARRY, VT, Expand);
----------------
This code comment won't age well. Delete.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34407
+// Optimize RES, EFLAGS = X86ISD::ADD LHS, RHS
+static SDValue combineADD(SDNode *N, SelectionDAG &DAG,
+ X86TargetLowering::DAGCombinerInfo &DCI) {
----------------
RKSimon wrote:
> We already have a combineAdd function - this should be named something like combineX86Add
+1 - we definitely do not want function names that only differ in captitalization.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34507
+ if (isAllOnesConstant(N->getOperand(1)) && N->hasAnyUseOfValue(1)) {
+ auto Carry = N->getOperand(0);
+ while (Carry.getOpcode() == ISD::TRUNCATE ||
----------------
auto -> SDValue
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34519
+ Carry.getOpcode() == X86ISD::SETCC_CARRY) {
+ auto Cond = cast<ConstantSDNode>(Carry.getOperand(0));
+ if (Cond->getZExtValue() == X86::COND_B)
----------------
auto -> auto *
https://reviews.llvm.org/D29872
More information about the llvm-commits
mailing list