This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] Improve handling of math functions.
ClosedPublic

Authored by jlebar on Aug 17 2016, 2:44 PM.

Details

Summary

A bunch of related changes here to our CUDA math headers.

  • The second arg to nexttoward is a double (well, technically, long double, but we don't have that), not a float.
  • Add a forward-declare of llround(float), which is defined in the CUDA headers. We need this for the same reason we need most of the other forward-declares: To prevent a constexpr function in our standard library from becoming host+device.
  • Add nexttowardf implementation.
  • Pull "foobarf" functions defined by the CUDA headers in the global namespace into namespace std. This lets you do e.g. std::sinf.
  • Add overloads for math functions accepting integer types. This lets you do e.g. std::sin(0) without having an ambiguity between the overload that takes a float and the one that takes a double.

With these changes, we pass testcases derived from libc++ for cmath and
math.h. We can check these testcases in to the test-suite once support
for CUDA lands there.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 68427.Aug 17 2016, 2:44 PM
jlebar retitled this revision from to [CUDA] Improve handling of math functions..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added a subscriber: cfe-commits.
tra added inline comments.Aug 17 2016, 3:21 PM
clang/lib/Headers/__clang_cuda_cmath.h
125–133 ↗(On Diff #68427)

You've got two identical nexttoward(float, double) now.
Perhaps first one was supposed to remain nexttoward(float, float) ?

184–197 ↗(On Diff #68427)

is_specialized will be true for long double args and we'll instantiate the function. Can we/should we produce an error instead?

jlebar added inline comments.Aug 17 2016, 4:27 PM
clang/lib/Headers/__clang_cuda_cmath.h
125–133 ↗(On Diff #68427)

It's hard to see, but one is nexttowardf.

184–197 ↗(On Diff #68427)

I think it's OK. Or at least, long double is kind of screwed up at the moment. Sometimes we pick __host__ overloads, sometimes we pick __device__ overloads; I made no effort to make it correct. I'm much more bullish on making use of long double a compile error as a way to solve these problems.

tra accepted this revision.Aug 17 2016, 5:04 PM
tra edited edge metadata.

LGTM, but we may want someone familiar with math library to take a look.

clang/lib/Headers/__clang_cuda_cmath.h
125–133 ↗(On Diff #68427)

Indeed, I've missed that.

This revision is now accepted and ready to land.Aug 17 2016, 5:04 PM
jlebar added a comment.EditedAug 18 2016, 1:49 PM

These changes have always been kind of scary. tra tested this against Thrust with all combinations of CUDA 7.0/7.5, c++98/11, libc++/libstdc++{4.8.5/4.9.3,5.3.0}. So we should be good here. I hope.

This revision was automatically updated to reflect the committed changes.