[PATCH] D62088: [compiler-rt][builtins] Scaled Integer log10()

Mon Jun 10 16:55:45 PDT 2019

leonardchan added a comment.

In D62088#1537081 <https://reviews.llvm.org/D62088#1537081>, @rjmccall wrote:

> In D62088#1531773 <https://reviews.llvm.org/D62088#1531773>, @leonardchan wrote:
>
> > > Well, I think the builtin has to be able to operate on signed scaled-integer / fixed-point types, and you'll have to specify the behavior for negative numbers.  If you're comfortable implementing those semantics inline in the caller, then I agree you don't need signed functions in compiler-rt.
> >
> > Ok. I'll add it to the caller when adding the builtin clang function. We can handle usage with different types there.
>
>
> Okay.  Please mention the behavior for 0 in the documentation, since this function is not otherwise defined for 0.

Done. We just return INT64/32_MIN for input of 0.

>>>> This could also be reasonably extended to support other sizes. I only chose a 64 bit size for now since it's the largest type we use and any other types could be resized/rescaled into this. For 128-bit, this would be a little more tricky since the algorithm depends on wide multiplication (which we kinda do through `x = (__uint128_t)x * x >> scale;`), although I can't seem to find an existing compiler-rt multiplication function that does this.
>>> 
>>> My concern about that is that it's going to limit this builtin to 64-bit targets — most 32-bit targets don't support `__uint128_t` (as an implementation concern), and there might be problems with using a 64-bit type in the parameter list.
>>> 
>>> As a separate concern, 128-bit multiplication might itself be implemented with a compiler-rt function on some targets, and I'm not sure how acceptable it is for compiler-rt functions to have this kind of cross-dependency.
>> 
>> I updated this patch and made a separate 32 bit version for this. The 64 bit version now uses `__uint128_t` only if compiler rt supports it, otherwise it uses a wide multiplication that I borrowed from `fp_lib.h`and a custom funnel shift right.
> 
> Thanks.  That sounds right to me, but I still don't feel qualified to actually review the approach. :)

No problem. @saugustine , would you feel comfortable reviewing this, or is there someone else you would recommend for compiler-rt?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62088