Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add many llvm float intrinsics. #2709

Merged
merged 5 commits into from
Jun 23, 2019
Merged

Add many llvm float intrinsics. #2709

merged 5 commits into from
Jun 23, 2019

Conversation

shawnl
Copy link
Contributor

@shawnl shawnl commented Jun 19, 2019

This adds a fused-multiply-add intrinsic to Zig. It turns out that LLVM emits calls to libm, and as such I had to add a basic libm implementation. Also, in debug mode f128 will fail to link, because there is no fmal implementation currently.

Also vector support requires quite a few bugs to be fixed.

Github is really bad at reviewing multiple patches, so please review these one at a time (and you can still leave comments), and DONT use github's review feature which makes it much harder to review.

@andrewrk
Copy link
Member

Nice work. I'd like to try to get this merged before my result location branch so that I can be the one to resolve the conflicts.

For the libm commit: I'd like to counter-propose that we unconditionally include libm inside libc. This is what musl does and I think it's a reasonable and simpler thing to do. As far as Zig is concerned, when you link libc, you get libm too. So we wouldn't need these extra --bundle-libm args. It can be as simple as adding the math functions to std/special/c.zig. Of course you can still introduce new files (such as std/special/libm.zig) and @import them from std/special/c.zig.

With #2062 solved (probably with the llvm equivalent of -ffunction-sections), plus the existing caching, I see no downsides to this approach.

@andrewrk
Copy link
Member

andrewrk commented Jun 19, 2019

As for the mulAdd commit, tests/stage1/behavior.zig needs to import your new test file otherwise the CI isn't running the new tests.

What bugs need to be fixed for vector support? I can look at those.

Another question I want to investigate is, what happens on a system where long double is not a 128-bit float? My understanding is that fmal is for long double, but in zig f128 floats are always 128 bit and do not necessarily correspond to long double. Ideally LLVM would emit a libcall to something for f128.

@shawnl
Copy link
Contributor Author

shawnl commented Jun 19, 2019

For the libm commit: I'd like to counter-propose that we unconditionally include libm inside libc.

I thought about this, and decided against it. (but can be persuaded otherwise) libm is generally useful, while libc is only needed to support c. For musl this is fine, because musl is a c library, and for glibc a seperate libm is stupid because libm depends on libc, but in our case we can actually ship a libm that does not depend on libc, which might be useful outside of zig.

Alternatively, I have no objections to putting this in compiler_rt. The problem there is that linking order becomes important because we cannot omit these symbols when linking against musl/glibc/other libc.

That said, --bundle-c should probably imply --bundle-libm.

What bugs need to be fixed for vector support?

#2708 #2707

Another question I want to investigate is, what happens on a system where long double is not a 128-bit float?

I wondered the same thing. LLVM emitted this for a llvm.fma.f128. The only platform I know where long double is possibly something else is PPC, and IBM's long double is a stupid and completely useless type (even if LLVM has support for it). In any case, we should only support IEEE-754 compliant types, and while that includes 80-bit (and also decimal float point), that isn't a priority right now.

@andrewrk
Copy link
Member

but in our case we can actually ship a libm that does not depend on [libc], which other might be useful outside of zig.

I'm questioning this. Can you provide an example use case?

Thanks I'll have a look at those bugs.

@andrewrk
Copy link
Member

Alternatively, I have no objections to putting this in compiler_rt

Both compiler_rt and zig's libc are required when linking zig code. The main difference between them is that the functions in libc are assumed to be provided by other libcs when linking against them. fma is clearly in the libc category, not the compiler_rt category.

@shawnl
Copy link
Contributor Author

shawnl commented Jun 19, 2019

Can you provide an example use case?

Web assembly doesn't really need a libc, <stdio.h> doesn't make much sense there. However, web assembly wants a libm. Nothing in libm depends on an OS, and there is not reason why it can't support <fenv.h> type stuff too. There at lots of places where complicated math is desired in the browser, even just drawing basic graphs.

And if glibc's libm worked without libc it would have been useful for zig, as it is highly optimzied.

@andrewrk
Copy link
Member

When Zig generates WebAssembly code, LLVM still emits libcalls to libc, for example memcpy. From Zig's perspective there is no difference between libm and libc; they are one and the same. What you are bringing up is this idea of "shims" which Zig does not (currently) do (for example provide fopen on webassembly). I believe that is a separate issue that will require a different solution than simply separating libc and libm.

@shawnl
Copy link
Contributor Author

shawnl commented Jun 19, 2019

I believe that is a separate issue that will require a different solution than simply separating libc and libm.

Ok, i'll put it in c.zig for now.

shawnl added 3 commits June 19, 2019 12:07
…tors of floats

Not all of the softfloat library is being built....

Vector support is very buggy at the moment, but should work when the bugs are fixed.
(as I had the same code working with another vector function, that hasn't been merged yet).
@shawnl
Copy link
Contributor Author

shawnl commented Jun 19, 2019

ugggh, github shows commits in chronological order instead of their tree (commit) order.

@shawnl
Copy link
Contributor Author

shawnl commented Jun 20, 2019

Can you look at this again? I wanted to implement a bunch more of these (and add vector support for some existing) very similar to this, so that later this won't slow me down as much.

And yes, once vectors are working better tests will be added, but as long as I add tests along the way, I think things should be good and I can move fast.

@shawnl shawnl changed the title Add @mulAdd intrinsic Add many llvm float intrinsics. Jun 21, 2019
@shawnl
Copy link
Contributor Author

shawnl commented Jun 21, 2019

I will port the new musl log/ln implementation (as I will need it) but it will be in a new pull request.

…trunc @round

and expand @sqrt

This revealed that the accuracy of ln is not as good as the current algorithm in
musl and glibc, and should be ported again.

v2: actually include tests
v3: fix reversal of in and out arguments on f128M_sqrt()
    add test for @sqrt on comptime_float
    do not include @nearbyInt() until it works on all targets.
@andrewrk andrewrk merged commit 71e014c into ziglang:master Jun 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants