[PATCH] D48498: [APInt] Add helpers for rounding u/sdivs.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/include/llvm/ADT/APInt.h:2160
 
+/// Return A unsign-divided by B, rounded by the given roundining mode.
+APInt RoundingUDiv(const APInt &A, const APInt &B, APInt::Rounding RM);
----------------
sanjoy wrote:
> Nit: rounding
> 
> Did you consider implementing these in `sdiv` and `udiv` and passing in a defaulted `Rounded` parameter?  That seems a bit cleaner to me.
> 
> Otherwise can you please add comments on `sdiv` and `udiv` about their rounding behavior?
> Did you consider implementing these in sdiv and udiv and passing in a defaulted Rounded parameter? That seems a bit cleaner to me.

I thought about it, but it seems bad to just add complexity to the current APInt API, especially when we didn't find more usage.

APIntOps, on the other hand, are considered by me a set of helpers built on the top of APInt, therefore not adding API complexity to APInt itself.

> Otherwise can you please add comments on sdiv and udiv about their rounding behavior?

I'm not sure if the behavior is intentionally not promised to the user, or just undocumented.


================
Comment at: llvm/lib/Support/APInt.cpp:2686
+    APInt Quo, Rem;
+    APInt::sdivrem(A, B, Quo, Rem);
+    if (Rem == 0)
----------------
sanjoy wrote:
> I'm wondering if this can be simplified.  Is this logic correct:
> 
> ```
> Q, R = sdivrem(A, B);
> if (R == 0) { return Q; }
> if (Q < 0) { return RoundDown ? Q-1:Q; }
> return RoundDown ? Q:Q-1;
> ```
> 
> the logic being that if R != 0 then the quotient is "off by one" depending on its sign.
I'm not sure, as the divisor maybe positive or negative. I found it difficult to prove.

Notice that the current algorithm is easy to prove, as it doesn't care whether sdiv rounds towards zero, or down, or up (I changed the comment to make that clear). As long as `Quo * B + Rem = A` holds, this algorithm is correct.


https://reviews.llvm.org/D48498