This is an archive of the discontinued LLVM Phabricator instance.

SimplifyLibCalls: Replace more unary libcalls with intrinsics
ClosedPublic

Authored by arsenm on Jan 9 2017, 2:23 PM.

Details

Reviewers
efriedma

Diff Detail

Event Timeline

arsenm updated this revision to Diff 83699.Jan 9 2017, 2:23 PM
arsenm retitled this revision from to SimplifyLibCalls: Replace more unary libcalls with intrinsics.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.
davide added a subscriber: davide.Jan 9 2017, 2:37 PM
davide added inline comments.
lib/Transforms/Utils/SimplifyLibCalls.cpp
2145–2155

Does your code handle double-float shrinking correctly? (just checking)

2222–2223

This can go away, right? If we lower to an intrinsic we should be able to constant fold it, no? (please add a test to verify)

2231–2232

Ditto.

arsenm updated this revision to Diff 83707.Jan 9 2017, 2:43 PM

Update comment

lib/Transforms/Utils/SimplifyLibCalls.cpp
2145–2155

The InstCombine part of the patch adds the shrinking with the intrinsics

For anyone else following along with a copy of IEEE 754, this is the roundToIntegral* family of functions.

Aren't llvm.rint() and llvm.nearbyint() precisely equivalent? We should probably get rid of llvm.rint()... (we can add it back once we actually start modeling FP exceptions).

test/Transforms/InstCombine/win-math.ll
273

Windows doesn't have round()? Weird.

davide added a comment.Jan 9 2017, 3:25 PM

For anyone else following along with a copy of IEEE 754, this is the roundToIntegral* family of functions.

Aren't llvm.rint() and llvm.nearbyint() precisely equivalent? We should probably get rid of llvm.rint()... (we can add it back once we actually start modeling FP exceptions).

I think so.

test/Transforms/InstCombine/win-math.ll
273

I'm surprised too https://msdn.microsoft.com/en-us/library/dn353646.aspx
This also compiles http://rextester.com/MUCW42464 (unless I'm misunderstanding).

For anyone else following along with a copy of IEEE 754, this is the roundToIntegral* family of functions.

Aren't llvm.rint() and llvm.nearbyint() precisely equivalent? We should probably get rid of llvm.rint()... (we can add it back once we actually start modeling FP exceptions).

Should I drop rint handling here or keep it until later when it's removed?

For anyone else following along with a copy of IEEE 754, this is the roundToIntegral* family of functions.

Aren't llvm.rint() and llvm.nearbyint() precisely equivalent? We should probably get rid of llvm.rint()... (we can add it back once we actually start modeling FP exceptions).

Should I drop rint handling here or keep it until later when it's removed?

I'd say keep it until we agree we're going to remove it.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1647

Why this newline.

I would say map @rint() to @llvm.nearbyint... but otherwise leave in the @llvm.rint() handling. We can kill off @llvm.rint() in a followup.

arsenm updated this revision to Diff 84618.Jan 16 2017, 5:47 PM

Only use nearbyint

efriedma accepted this revision.Jan 23 2017, 1:57 PM

The intrinsics aren't used nearly as much as the function calls, so it's possible this will expose issues... but that's fine, I think.

This revision is now accepted and ready to land.Jan 23 2017, 1:57 PM
arsenm closed this revision.Jan 23 2017, 4:07 PM

r292855