[PATCH] D60522: [KnownBits] Add computeForAddCarry()

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:20-21
+    const KnownBits &LHS, const KnownBits &RHS,
+    bool CarryZero, bool CarryOne) {
+  assert(!(CarryZero && CarryOne) &&
+         "Carry can't be zero and one at the same time");
----------------
bjope wrote:
> nikic wrote:
> > lebedev.ri wrote:
> > > How about simply making it `KnownBits(/*KnownBits=*/1)` ?
> > It's a possibility, but I'm not convinced it's better here. The booleans directly enforce the contract that this is just one bit (while for KnownBits we could only assert that), they're cheaper to construct and use than two APInts, and the API is simpler to use, because we don't have to construct KnownBits (which is somewhat roundabout due to lack of a direct ctor).
> > 
> > I think the only place that would really benefit from passing carry as KnownBits is the testing code, while for other usages having this as booleans is more convenient and efficient.
> If we want to also track the carry in the future I guess we would get that as a "KnownBits".
> 
> So I think that for ADDCARRY we probably want to do
> ```
>   KnownBits KnownLHS = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
>   KnownBits KnownRHS = computeKnownBits(Op.getOperand(1), DemandedElts, Depth + 1);
>   KnownBits KnownCarry = computeKnownBits(Op.getOperand(2), DemandedElts, Depth + 1);
>   Known = computeForAddCarry(KnownLHS, KnownRHS, KnownCarry.trunc(1));
> ```
> if we also want to also track down the known bits of the carry.
> 
> So having a `computeForAddCarry` method that takes a `KnownBits(1)` might actually be useful (compared to trying to convert `KnownCarry` into two booleans on the user side of this API). If there is a use for also having the bool based interface, then perhaps we can have both?
That's a good point. I've changed the implementation to use a 1-bit KnownBits in the exported API. The variant that uses two booleans is now only used internally to shared between the exported computeForAddCarry and computeForAddSub APIs.


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

https://reviews.llvm.org/D60522