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

[Zvi Rackover via Phabricator via llvm-commits]

zvi 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)))
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D29268