[PATCH] D29268: [DAGCombine] Combine composition of ADDC(ADDE)

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1872
+  if (N1.getOpcode() == ISD::ADDE &&
+      N->isOnlyUserOf(N1.getValue(0).getNode()) &&
+      isNullConstant(N1.getOperand(1)))
----------------
zvi wrote:
> deadalnix wrote:
> > deadalnix wrote:
> > > zvi wrote:
> > > > zvi wrote:
> > > > > deadalnix wrote:
> > > > > > zvi wrote:
> > > > > > > RKSimon wrote:
> > > > > > > > zvi wrote:
> > > > > > > > > deadalnix wrote:
> > > > > > > > > > Is that important to check this has only one use ? I'd assume that having (adde Y, 0, Carry) + (adde X, Y, Carry) is still preferable to (addc X, (adde Y, 0, Carry)) as it break dependencies between instructions.
> > > > > > > > > That's a fair point, but here I am following the practice that is common in the DAG Combiner. If you search for "hasOneUse" or "isOnlyUserOf" you can see the conservative approach of avoiding increase in instruction count.
> > > > > > > > > If you can provide a benchmark that can show this case is an exception, we can discuss the trade-off with data to guide us - and better do it in a follow-up patch.
> > > > > > > > Are there any situations where the inner ADDE's carry flag will be used by something else?
> > > > > > > The carry's type is glue, so by definition it can have a single user, right?
> > > > > > > If you search for "hasOneUse" or "isOnlyUserOf" you can see the conservative approach of avoiding increase in instruction count.
> > > > > > 
> > > > > > I don't think this is a valid concern in that case. If the instruction has more than one use, you get 2 instructions before and 2 after. So I don't think this apply in that case.
> > > > > > 
> > > > > > However, I'm not sure about the glue. It may indeed be the case that it is expected that a glue is used only once (that would make sense). In which case maybe a adding comment is a good thing because if it is not obvious to us now, it won't be to some dude reading this in a few month without context.
> > > > > You can see for yourself the results of your suggestion by applying this patch and running 'make check-llvm'.
> > > > > At least the tests 'mul-i1024.ll' and 'mul-i512.ll' will be affected by this change. You will see the instruction count will increase. Not saying this is necessarily undesirable, but it requires additional work of convincing ourselves that the larger code is faster.
> > > > Sorry, it would have been clearer if i wrote: ".. by applying this patch only without the condtion of '&&
> > > >       N->isOnlyUserOf(N1.getValue(0).getNode())'
> > > Alright, I think this is moot anyway. I'm not sure this transformation is valid if Y may be all ones. adde would generate a carry, but the addc would not, after the transformation, it would generate a carry.
> > I suggest something like:
> > 
> >   // (addc X, (adde Y, 0, Carry)) -> (adde X, Y, Carry) if Y + 1 cannot overflow.
> >   if (N1.getOpcode() == ISD::ADDE &&
> >       N->isOnlyUserOf(N1.getValue(0).getNode()) &&
> >       isNullConstant(N1.getOperand(1))) {
> >     APInt YZero, YOne;
> >     DAG.computeKnownBits(N1.getOperand(0), YZero, YOne);
> >     if (YZero)
> >       return DAG.getNode(ISD::ADDE, SDLoc(N), N->getVTList(), N0,
> >                          N1->getOperand(0), N1->getOperand(2));
> >   }
> > 
> Good catch. Thanks
For what it's worth, in the meantime, I did https://reviews.llvm.org/D29443 which solve the whole thing. This pattern is part of the solution.


Repository:
  rL LLVM

https://reviews.llvm.org/D29268