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

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In D62088#1531324 <https://reviews.llvm.org/D62088#1531324>, @leonardchan wrote:

> In D62088#1523994 <https://reviews.llvm.org/D62088#1523994>, @saugustine wrote:
>
> > The number is the argument count plus one for return value. So this function should be named
> >
> > __log10scaleddi3
>
>
> Done and clang-formated
>
> In D62088#1531310 <https://reviews.llvm.org/D62088#1531310>, @rjmccall wrote:
>
> > In D62088#1531309 <https://reviews.llvm.org/D62088#1531309>, @leonardchan wrote:
> >
> > > In D62088#1522446 <https://reviews.llvm.org/D62088#1522446>, @rjmccall wrote:
> > >
> > > > I don't review compiler-rt patches generally.  Can you explain what would actually use this function?  Is there supposed to be a new Clang builtin for it?
> > >
> > >
> > > We will be using this as part of our WLAN library which represents energy units (decibels and watts) as scaled integers that we have custom classes for. Converting between these units and adding decibels mathematically involves using log10(). Currently, we just use float log10 and casting, but would like to use this for scaled integers and eventually fixed point types. The idea is that this would be a new clang builtin that we could use instead of a floating point log10().
> >
> >
> > Okay.  Will you need a signed variant of this?  Is using a 64-bit argument reasonable for all targets, or should there be 32-bit and 64-bit (and maybe eventually 128-bit) variants?
>
>
> I don't think we need a signed version since ideally all inputs to log functions should be non-negative, otherwise the result is imaginary.


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.

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62088