Page MenuHomePhabricator

[compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible.
ClosedPublic

Authored by rupprecht on Jul 18 2018, 3:04 PM.

Details

Summary

The complex division builtins (div?c3) use logb methods from libm to scale numbers during division and avoid rounding issues. However, these come from libm, meaning anyone that uses --rtlib=compiler-rt also has to include -lm. Implement logb* methods for standard ieee 754 floats so we can avoid -lm on those platforms, falling back to the old behavior (using either logb() or __builtin_logb()) when not supported.

These new methods are defined internally as __compiler_rt_logb so as not to conflict with the libm definitions in any way.

This fixes just the libm methods mentioned in PR32279 and PR28652. libc is still required, although that seems to not be an issue.

Note: this is proposed as an alternative to just adding -lm: D49330.

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Jul 18 2018, 3:04 PM
Herald added subscribers: Restricted Project, llvm-commits, delcypher and 4 others. · View Herald TranscriptJul 18 2018, 3:04 PM

Ping -- Eli, do you have time to review this, or add someone else that can take a look if you're busy?

Maybe just make this a static inline function in fp_lib.h?

Maybe worth implementing something like ilogb instead, to avoid an extra int->float->int conversion? I guess it's not that important.

Fixes PR32279 and PR28652.

Is this really enough to fix PR32279? That report mentions some other symbols (scalbn*, fmaxl).

lib/builtins/ppc/divtc3.c
26 ↗(On Diff #156161)

This is wrong. The original code is calling logb, not logbl.

efriedma edited reviewers, added: compnerd, howard.hinnant; removed: saugustine.Jul 23 2018, 5:25 PM
rupprecht marked an inline comment as done.Jul 25 2018, 12:35 PM

Maybe just make this a static inline function in fp_lib.h?

Thanks, done. That also makes these symbols not exported by the library, which is nice.

Maybe worth implementing something like ilogb instead, to avoid an extra int->float->int conversion? I guess it's not that important.

That sounds like a good idea, although the change is a little more invasive (e.g. all the div methods check against logb == infinity). I'd prefer to do that as a followup change if that's OK.

Fixes PR32279 and PR28652.

Is this really enough to fix PR32279? That report mentions some other symbols (scalbn*, fmaxl).

Hmm, this fixes it enough for us, but you're right this isn't providing scalbn*/fmax*. We must be adding a -lc automatically somewhere, which provides those methods. I'll update the patch description that this is only a partial fix.

lib/builtins/ppc/divtc3.c
26 ↗(On Diff #156161)

This is wrong. The original code is calling logb, not logbl.

Yes, you're right.
I must have used logbl here because this is providing complex division for long doubles (c.f. non-ppc divtc3 uses logbl). But despite that, the method used is (non-long) doubles, due to ppc's floating point format here. I've fixed to use logb and left a comment to avoid that confusion.

rupprecht marked an inline comment as done.
  • Inline methods to fp_lib.h and fix the ppc logb call
rupprecht edited the summary of this revision. (Show Details)Jul 25 2018, 12:41 PM
efriedma added inline comments.Jul 25 2018, 12:50 PM
lib/builtins/divxc3.c
16 ↗(On Diff #157339)

Could you use a define that isn't QUAD_PRECISION here? Or just skip changing this file for now? This is the x86 80-bit long double.

lib/builtins/ppc/divtc3.c
8 ↗(On Diff #157339)

Please change the comment so it doesn't refer to QUAD_PRECISION; that's specifically an IEEE 128-bit float, which isn't relevant here.

rupprecht marked 3 inline comments as done.Jul 25 2018, 1:11 PM
rupprecht added inline comments.
lib/builtins/divxc3.c
16 ↗(On Diff #157339)

Could you use a define that isn't QUAD_PRECISION here? Or just skip changing this file for now? This is the x86 80-bit long double.

80-bit long double doesn't fit the abstractions used in fp_lib.h, so I'll just skip for now.

rupprecht updated this revision to Diff 157347.Jul 25 2018, 1:11 PM
rupprecht marked an inline comment as done.
  • Exclude 80-bit floating point and fix ppc comment
rupprecht updated this revision to Diff 160020.Aug 9 2018, 3:16 PM
rupprecht marked an inline comment as done.
  • Un-revert divtc3 (128-bit fp), revert divxc3 (80-bit fp)
lib/builtins/divxc3.c
16 ↗(On Diff #157339)

Just realized I reverted the wrong one -- fixed so that I'm skipping divxc3 in this revision, but including divtc3 (both generic and ppc versions)

LGTM, but I'd feel more comfortable if a second set of eyes looked at this.

Ping for @howard.hinnant @compnerd -- mind taking a quick look for a second LGTM?

rupprecht edited the summary of this revision. (Show Details)Sep 18 2018, 4:18 PM

@compnerd, can you take a look? Or add another reviewer comfortable approving if you can't review?

Adding Steve here in case he wants to opine.

compnerd added inline comments.Sep 21 2018, 1:20 PM
lib/builtins/fp_lib.h
274 ↗(On Diff #160020)

I don't see why we need __inline, I'm pretty sure that we compile in C99 mode, so inline should be fine. Doesn't this require GNU semantics though? Did you try building the windows target with this change?

302 ↗(On Diff #160020)

Is this taken from somewhere else or is this code written from scratch?

323 ↗(On Diff #160020)

Why the fallback to libm?

Additionally, the names are different, so is there a reason to not just define the variants unconditionally?

rupprecht added inline comments.Sep 21 2018, 2:47 PM
lib/builtins/fp_lib.h
274 ↗(On Diff #160020)

This is consistent with all the other methods in this file.

Actually, it looks like the change from inline to __inline was from your revision: https://reviews.llvm.org/rL249953

302 ↗(On Diff #160020)

This is written from scratch.

323 ↗(On Diff #160020)

Why the fallback to libm?

This generic implementation only makes sense for ieee754, but some of the compiler-rt methods use other floating point types (e.g. divxc3 uses x86 80-bit long doubles). Hence, this implementation is incomplete and is using libm for now. (Note: this is not adding a dependency on libm, this is just calling out an existing one).

Additionally, the names are different, so is there a reason to not just define the variants unconditionally?

I originally had it split up like that (history link -- https://reviews.llvm.org/D49514?vs=on&id=156161&whitespace=ignore-most#toc -- things are split up into compiler_rt_logb.c etc.), but Eli suggested making it inline in an earlier comment. I think this way is a little nicer, but I don't have a strong opinion.

rupprecht updated this revision to Diff 166563.Sep 21 2018, 2:48 PM
  • Elaborate libm dependency in comment

This seems reasonable to me, however, I would want to give @scanon a few (work) days to see if he has any comments on the actual implementation of logb.

I suspect that we'd rather use ilogb in the long term, but as a like-for-like replacement this looks OK.

echristo accepted this revision.Sep 24 2018, 11:27 AM

Sounds like everyone is in favor. Hitting the accept button for Jordan :)

This revision is now accepted and ready to land.Sep 24 2018, 11:27 AM
This revision was automatically updated to reflect the committed changes.

It seems that http://lab.llvm.org:8011/builders/clang-x64-ninja-win7 buildbot is failing because of your commit.

I've disabled the tests on Windows in r343095.