This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Compile __powitf2 under wasm
ClosedPublic

Authored by sbc100 on Feb 7 2020, 8:37 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Feb 7 2020, 8:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 7 2020, 8:37 PM
Herald added subscribers: llvm-commits, Restricted Project, sunfish and 3 others. · View Herald Transcript

Any idea why this file was listed under x86_ARCH_SOURCES? But only seems to active for ARCH_PPC?

I think the reason we noticed this being missing under wasm is because we (somewhat unusually) use fp128 for long double

phosek accepted this revision.Feb 9 2020, 3:46 PM

Any idea why this file was listed under x86_ARCH_SOURCES? But only seems to active for ARCH_PPC?

That looks like a bug to me.

This revision is now accepted and ready to land.Feb 9 2020, 3:46 PM
efriedma requested changes to this revision.Feb 9 2020, 4:58 PM
efriedma added a subscriber: efriedma.
efriedma added inline comments.
compiler-rt/lib/builtins/CMakeLists.txt
147 ↗(On Diff #243345)

powixf2.c is, in fact, x86-specific. At first glance it might not look like that, but the "x" in the name powixf2 refers exclusively to x86 long double.

compiler-rt/lib/builtins/powitf2.c
16

Please change this so it matches the other 128 long double routines: it should include fp_lib.h, it should use the condition #if defined(CRT_HAS_128BIT) && defined(CRT_LDBL_128BIT), and it should use fp_t instead of explicitly writing out "long double".

This revision now requires changes to proceed.Feb 9 2020, 4:58 PM
sbc100 updated this revision to Diff 243718.Feb 10 2020, 7:36 PM
sbc100 marked 2 inline comments as done.
  • revert cmake
  • feedback
sbc100 edited the summary of this revision. (Show Details)Feb 10 2020, 7:36 PM
sbc100 added inline comments.Feb 10 2020, 7:48 PM
compiler-rt/lib/builtins/CMakeLists.txt
147 ↗(On Diff #243345)

Indeed, sorry I misread/misunderstood.

compiler-rt/lib/builtins/powitf2.c
16

Great those macros looks like what I'm looking for.

Would you mind doing the fp_t change as a followup? I don't know this codebase well enough to feel confident doing that part.

This revision is now accepted and ready to land.Feb 11 2020, 2:04 PM
This revision was automatically updated to reflect the committed changes.