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


Repository:
  rL LLVM

https://reviews.llvm.org/D29268