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

Amaury SECHET via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 1 23:51:19 PST 2017

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:
> > 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.
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.



More information about the llvm-commits mailing list