[PATCH] D107692: [DAGCombine] Prevent the transform of combine for multi-use operand

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4944
   if (N0.getOpcode() == ISD::ADD && N1.getOpcode() == ISD::SRL &&
-      VT.getSizeInBits() <= 64) {
+      VT.getSizeInBits() <= 64 && N0.hasOneUse()) {
     if (ConstantSDNode *ADDI = dyn_cast<ConstantSDNode>(N0.getOperand(1))) {
----------------
craig.topper wrote:
> lebedev.ri wrote:
> > dmgreen wrote:
> > > N0->hasOneUse() would probably be better, to check the Node has one use not the SDValue. (Although here it likely won't make much difference.)
> > *Why* does it matter for the correctness whether the node has other uses or not?
> > The peephole should only affect the current root use we started with,
> > and not affect any other uses whatsoever.
> There is a CombineTo call on N0. That will affect all users of N0.
Aha, that indeed explains it, and should have been stated in the patch's description, thanks.
But why is it there?
Sounds like this transform should instead be
```
            if (TLI.isLegalAddImmediate(ADDC.getSExtValue())) {
              SDLoc DL0(N0);
              SDValue NewAdd =
                DAG.getNode(ISD::ADD, DL0, VT,
                            N0.getOperand(0), DAG.getConstant(ADDC, DL, VT));
              return DAG.getNode(ISD::AND, DL0, VT, NewAdd, N1);
            }
```


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

https://reviews.llvm.org/D107692