This is an archive of the discontinued LLVM Phabricator instance.

SimplifyLibCalls: Replace fabs libcalls with intrinsics
ClosedPublic

Authored by arsenm on Jan 6 2017, 11:34 AM.

Details

Reviewers
efriedma
Summary

Add missing fabs(fpext) optimzation that worked with the call,
and also fixes it creating a second fpext when there were multiple
uses.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 83393.Jan 6 2017, 11:34 AM
arsenm retitled this revision from to SimplifyLibCalls: Replace fabs libcalls with intrinsics.
arsenm updated this object.
arsenm added a reviewer: efriedma.
arsenm added a subscriber: llvm-commits.
efriedma edited edge metadata.Jan 6 2017, 12:12 PM

Canonicalizing @fabs() -> @llvm.fabs() makes sense.

I'd like to see a regression test which specifically checks the fabs/fabsf/fabsl -> llvm.fabs transform. Please include testcases which transform fabsl -> llvm.fabs.f64, fabsl -> llvm.fabs.f80, and fabsl -> llvm.fabs.f128, to make sure they all work as expected.

Please make sure to kill off all the existing optimizations besides this one which check for LibFunc::fabs or the string "fabsf" in a followup.

test/Transforms/InstCombine/double-float-shrink-2.ll
27

"replacable" is a little unclear... I think the point is that the intrinsic does the right thing on all platforms?

test/Transforms/InstCombine/fabs.ll
19–20

This is probably going to fail on Windows... maybe change the test to call llvm.fabs rather than fabs?

arsenm updated this revision to Diff 83531.Jan 7 2017, 11:45 AM
arsenm edited edge metadata.

Test adjustments

arsenm updated this revision to Diff 83590.Jan 8 2017, 11:40 PM

Preserve fast math flags

efriedma added inline comments.Jan 9 2017, 12:03 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1642

Do we need to preserve fast-math flags here?

lib/Transforms/Utils/SimplifyLibCalls.cpp
1221

Do we need to preserve fast-math flags here?

test/Transforms/InstCombine/double-float-shrink-2.ll
91

We allow putting fast-math flags on arbitrary calls...? Please update LangRef with an explanation of what this means.

arsenm added inline comments.Jan 9 2017, 12:11 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1642

I don't think fpext supports fast math flags

arsenm added inline comments.Jan 9 2017, 12:16 PM
test/Transforms/InstCombine/double-float-shrink-2.ll
91

FMF on calls was added in r255555

arsenm added inline comments.Jan 9 2017, 12:19 PM
test/Transforms/InstCombine/double-float-shrink-2.ll
91

This seems to already be noted in the langref for call

arsenm updated this revision to Diff 83667.Jan 9 2017, 12:30 PM

Preserve fast math flags

efriedma added inline comments.Jan 9 2017, 12:39 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1642

Sorry, I meant fast-math flags on the call.

test/Transforms/InstCombine/double-float-shrink-2.ll
91

Oh, I was looking in the wrong place; it looks like there's a missing note in the section on fast-math flags.

arsenm updated this revision to Diff 83702.Jan 9 2017, 2:30 PM

Minor test improvement

Anything else?

I'd still like to see a test which checks the transform fabsl() -> llvm.fabs.f80().

lib/Transforms/InstCombine/InstCombineCalls.cpp
1640

Fast-math flags?

test/Transforms/InstCombine/fabs.ll
19–20

Still probably going to fail on Windows, where fabsf() doesn't exist.

arsenm added inline comments.Jan 16 2017, 1:24 PM
test/Transforms/InstCombine/fabs.ll
19–20

Do you mean the intrinsic won't work? I would expect codegen to promote it

arsenm updated this revision to Diff 84588.Jan 16 2017, 1:34 PM

Test fp80

test/Transforms/InstCombine/fabs.ll
19–20

It looks like a fabs instruction is emitted for the intrinsic

efriedma added inline comments.Jan 16 2017, 1:43 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1640

Fast-math flags?

test/Transforms/InstCombine/fabs.ll
19–20

No... I mean we call TLI.setUnavailable(LibFunc::fabsf); on Windows, so it isn't a library call, so we'll never call optimizeFabs.

arsenm added inline comments.Jan 16 2017, 1:45 PM
test/Transforms/InstCombine/fabs.ll
19–20

I thought the intent there was to not recognize fabsf as a special builtin function since it isn't available, i.e. fabsf could be a user function

efriedma added inline comments.Jan 16 2017, 1:48 PM
test/Transforms/InstCombine/fabs.ll
19–20

Yes, that's working as intended; the only problem is that you didn't specify a triple for this test, so it will inherit the triple from the host, therefore on a Windows host the CHECK line won't match.

arsenm updated this revision to Diff 84591.Jan 16 2017, 1:55 PM

Preserve fast math flags in another place

arsenm updated this revision to Diff 84592.Jan 16 2017, 1:57 PM

Add triple to test

efriedma accepted this revision.Jan 16 2017, 2:28 PM

LGTM.

This revision is now accepted and ready to land.Jan 16 2017, 2:28 PM
arsenm closed this revision.Jan 16 2017, 4:21 PM

r292172