[PATCH] D64174: [DAGCombine] Do several rounds of combine for addcarry nodes.

Sat Jul 20 18:37:42 PDT 2019

deadalnix marked an inline comment as done.
deadalnix added inline comments.

================
Comment at: test/CodeGen/X86/addcarry.ll:326
+; CHECK-NEXT:    adcq %rdi, %rbx
+; CHECK-NEXT:    addq %r10, %rdx
+; CHECK-NEXT:    adcq %rdi, %rcx
----------------
RKSimon wrote:
> craig.topper wrote:
> > RKSimon wrote:
> > > deadalnix wrote:
> > > > craig.topper wrote:
> > > > > Doesn't this add and adc compute the same result as line 321 and 325?
> > > > There is a lot of duplication that is generated by this. But once the carry propagation is "linearized" because you removed all the diamonds, then a simple set of optimization can get rid of it all. See D57317 for that specific case.
> > > Would adding X86ISD::ADD to X86TargetLowering::isCommutativeBinOp help with this?
> > If I remember right from what I saw in the DAG. We need to CSE ISD::ADDCARRY with commuted operands in DAG combine. Not sure about the X86ISD node.
> By the looks of this, DAGCombiner::combine should handle it - but is disabled for commutative binop nodes with more than 1 output value.....
@RKSimon It doesn't help, because ADDCARRY is not a binary op. It has 3 inputs, 2 of which are commutative. However, D57317 is ready to go and address that exact problem. I did not land D57317 because as far as I know, this doesn't happen in the wild without this patch.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64174