This is an archive of the discontinued LLVM Phabricator instance.

[X86] Implement llvm.isnan(x86_fp80) as unordered comparison
ClosedPublic

Authored by sepavloff on Aug 13 2021, 7:32 AM.

Details

Summary

x86_fp80 format allows values that do not fit any of IEEE-754 category.
Previously they were recognized by intrinsic __builtin_isnan as NaNs.
Now this intrinsic is implemented using instruction FXAM, which
distinguish between NaNs and unsupported values. It can make some
programs behave differently.

As a solution, this fix changes lowering of the intrinsic. If floating
point exceptions are ignored, llvm.isnan is lowered into unordered
comparison, as __buildtin_isnan was implemented earlier. In strictfp
functions the intrinsic is lowered using FXAM, which does not raise
exceptions even for signaling NaN, as required by IEEE-754 and C
standards.

Diff Detail

Event Timeline

sepavloff created this revision.Aug 13 2021, 7:32 AM
sepavloff requested review of this revision.Aug 13 2021, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2021, 7:32 AM
thopre added a subscriber: thopre.Aug 13 2021, 7:55 AM

Thanks for following up. I am not qualified to review this patch. But, one question: why not use fxam in all cases?

dxf added a subscriber: dxf.Aug 13 2021, 12:36 PM
kpn added a subscriber: kpn.Aug 13 2021, 1:05 PM

But, one question: why not use fxam in all cases?

Because it's more instructions?

As a solution, this fix changes lowering of the intrinsic. If floating
point exceptions are ignored, llvm.isnan is lowered into unordered
comparison, as __buildtin_isnan was implemented earlier. In strictfp
functions the intrinsic is lowered using FXAM, which does not raise
exceptions even for signaling NaN, as required by IEEE-754 and C
standards.

This seems like only half of a solution. If we actually care about this, we should change the way we check the result of fxam. ("unsupported" is one of the categories returned by fxam.)

I'm not convinced we care about this, though. We could probably just say in LangRef that floating-point operations on "unsupported" x86_fp80 values produce poison, and nobody would notice the difference.

I'm not convinced we care about this, though. We could probably just say in LangRef that floating-point operations on "unsupported" x86_fp80 values produce poison, and nobody would notice the difference.

Its probably debatable whether one should care about the unsupported numbers. But, the reason for my comment on the original change was because we actually noticed the difference :)

Also, other commonly used floating point libraries like the GNU MPFR library treat the unsupported numbers as NaNs.

kpn added a comment.Aug 13 2021, 1:57 PM

I'm not sure I like the idea of having the compiler know exceptions may be enabled change the results of isnan(). It's subtle and can result in surprising behavior if, for example, a function that called isnan() is inlined into a function that uses the constrained intrinsics with "maytrap" or "strict".

sepavloff updated this revision to Diff 366605.Aug 16 2021, 5:43 AM

Treat unsupported values as NaNs

sivachandra accepted this revision.Aug 18 2021, 11:33 AM

I think the current version of the patch restores the previous behavior so should be good to go. I am accepting but I am also not an expert in the area touched here. So, I will let the author @sepavloff decide how they want to proceed.

Also, I would like to understand why we cannot use fxam for all cases. For, exceptions can be enabled and disabled from software and should not affect the behavior of isnan.

This revision is now accepted and ready to land.Aug 18 2021, 11:33 AM

Thanks!

I will wait a couple of days, in case some reviewers have additional notes.

Also, I would like to understand why we cannot use fxam for all cases. For, exceptions can be enabled and disabled from software and should not affect the behavior of isnan.

For x86_fp80 it is used in all cases. For double and float there are more compact and faster solutions.

This revision was landed with ongoing or failed builds.Aug 27 2021, 4:09 AM
This revision was automatically updated to reflect the committed changes.