[PATCH] D120327: compiler-rt: Add udivmodei5 to builtins and add bitint library

Wed Feb 23 10:54:30 PST 2022

programmerjake added a comment.

In D120327#3339898 <https://reviews.llvm.org/D120327#3339898>, @mgehre-amd wrote:

> In D120327#3338323 <https://reviews.llvm.org/D120327#3338323>, @programmerjake wrote:
>
>> As far as I can tell, this code seems to assume it's running on a little-endian system. I think it needs to correctly handle what happens when bigints are stored in the other endian (where the first word is the most-significant).
>
> Yes, that's a good observation. I don't have access to a big-endian system right now. Should I disable building those functions on big-endian systems for this PR?

probably...

>> Also, the division code seems to assume that the most-significant word is always non-zero or the whole value is zero, which would allow it to use the number of words as a proxy for how big the value is numerically, which isn't the case.
>
> Could you please tell me why you think that? I don't believe it's true, and I think the tests in divmodei5_test.c show that,
> specifically `if (test__divmodei5(2, 1, 2, 0, nbits, a_scratch, b_scratch, quo_scratch, rem_scratch))` does a 2/1 division on integers of various sizes.

I left an inline comment on one such place.

================
Comment at: compiler-rt/lib/builtins/udivmodei5.c:292
+  // special case: dividend is too small so quotient is always zero
+  if (lhsWords < rhsWords) {
+    if (Quotient) {
----------------
here's one place where the number of words is incorrectly assumed to be some indicator of numeric value

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120327