Page MenuHomePhabricator

Remove *_finite library support, following upstream

Authored by serge-sans-paille on Feb 17 2020, 6:31 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 6:31 AM

Update some tests

I'm a little concerned we're dropping the code to recognize calls in the source code too soon. Existing headers will generate calls to *_finite with fast-math, and people will continue to use existing headers for the next few years, until everyone updates their distro/sysroot.

I'm fine getting rid of the code to lower the intrinsics to *_finite calls if they're not actually faster in practice.

I'm fine getting rid of the code to lower the intrinsics to *_finite calls if they're not actually faster in practice.

Well the problem is rather that they're no longer available in libm. I get your point though, and will limit that patch to the lowering step.

As hinted by @efriedma , only remove lowering to *_finite calls in Codegen.

I contacted the libm maintainer and here is his explanation:

They were only ever an ABI (selected by use of -ffinite-math-only or
options implying it, which resulted in the headers using "asm" to redirect
calls to some libm functions), not an API. The change means that ABI has
turned into compat symbols (only available for existing binaries, not for
anything newly linked, not included in static libm at all, not included in
shared libm for future glibc ports such as RV32), so, yes, in any case
where tools generate direct calls to those functions (rather than just
following the "asm" annotations on function declarations in the headers),
they need to stop doing so.

Code changes are what I expect, yes.

Missing test changes?

It's a little weird that we're still recognizing these as LibFuncs, but I guess that's necessary to keep the optimizations like constant folding working with them. I see that ConstantFolding already calls the non-finite-restricted versions of the functions when it's doing compile-time evaluation. It would be good to have a comment somewhere (probably in TargetLibraryInfo.cpp below the note about math-finite.h) explaining the handling of these functions.

serge-sans-paille retitled this revision from Remove *_finite libary support, following upstream to Remove *_finite library support, following upstream.

@andrew.w.kaylor / @efriedma : comments taken into account.

efriedma accepted this revision.Feb 27 2020, 9:43 AM

LGTM with one minor comment.


You probably don't need the ANDROID-AARCH64 lines in the test anymore. Maybe just delete them, and make the Android run-line use the same CHECK lines?

This revision is now accepted and ready to land.Feb 27 2020, 9:43 AM

Update test case, according to @efriedma suggestion.

This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Feb 28 2020, 2:52 AM

Please include the PR number next time :-) PR45034 in this case.