[PATCH] D60460: [SelectionDAG] Use KnownBits::computeForAddSub in SelectionDAG::computeKnownBits

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2948
         Known.Zero.setBits(1, KnownZeroLow);
       break;
     }
----------------
bjope wrote:
> nikic wrote:
> > I think it would make more sense to move the computeForAddSub calculation above this branch, and then do something like this here:
> > 
> > ```
> > // With ADDE and ADDCARRY a carry bit may be added, so we can only use
> > // this information if we know that the low bit is clear.
> > if (!lowBitSet(Known.Zero)) { // How do you write this operation with APInt?
> >   Known.One.clearAllBits();
> >   Known.Zero.clearAllBits();
> >   break;
> > }
> > 
> > Known.Zero.clearBit(0);
> > ```
> > 
> > This allows us to use the full known bits result, just without the low bit, if it is possible.
> Or maybe I can use computeForAddSub a second time, simulating an add of the carry bit
> ```
>     // With ADDE and ADDCARRY, a carry bit may be added in.
>     if (Opcode == ISD::ADDE || Opcode == ISD::ADDCARRY) {
>       // TODO: Can we compute known bits for the carry bit? For now we set it to
>       // unknown.
>       Known2.setAllZero();
>       Known2.Zero.clearBit(0);
>       Known = KnownBits::computeForAddSub(/* IsAdd */ true, /* NSW */ false,
>                                           Known, Known2);
>     }
> 
> ```
This works and is fully accurate. I think it would be slightly better to integrate the carry in the computeForAddSub() calculation, because it's cheaper than doing it twice. I've opened D60522 to support this. With this method the implementation here would reduce to:

```
bool CarryKnownZero = Opcode != ISD::ADDE && Opcode != ISD::ADDCARRY;
Known = KnownBits::computeForAddCarry(
    Known, Known2, CarryKnownZero, /*CarryKnownOne*/ false);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60460