This is an archive of the discontinued LLVM Phabricator instance.

Use llvm.is_fpclass to implement FP classification functions
ClosedPublic

Authored by sepavloff on Nov 1 2021, 8:31 AM.

Details

Summary

Builtin floating-point number classification functions:

  • __builtin_isnan,
  • __builtin_isinf,
  • __builtin_finite, and
  • __builtin_isnormal

now are implemented using llvm.is_fpclass. New builtin functions were
added:

  • __builtin_issubnormal,
  • __builtin_iszero, and
  • __builtin_issignaling.

They represent corresponding standard C library functions and are also
implemented using llvm.is_fpclass.

A new builting function __builtin_isfpclass is added. It is called as:

  • __builtin_isfpclass(<test>, <floating point value>)

and returns an integer value, which is non-zero if the floating point
argument belongs to one of the classes specified by the first argument,
and zero otherwise. The set of classes is an integer value, where each
value class is represented by a bit. There are ten data classes, as
defined by the IEEE-754 standard.

This change makes the target callback TargetCodeGenInfo::testFPKind
unneeded. It is preserved in this change and should be removed later.

Diff Detail

Event Timeline

sepavloff requested review of this revision.Nov 1 2021, 8:31 AM
sepavloff created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 8:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sepavloff updated this revision to Diff 410303.Feb 21 2022, 7:42 AM

Updated patch because the base revision is updated

qiucf added a subscriber: qiucf.Dec 8 2022, 7:11 AM

This patch looks good and llvm.is.fpclass will by default be expanded (except SystemZ which has their own lowering). Is there any blocker for this?

Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 7:11 AM
sepavloff updated this revision to Diff 481901.Dec 11 2022, 3:31 AM

Prepare the patch for review

In this patch the callback TargetCodeGenInfo::testFPKind is
preserved, to make this change safer.

sepavloff retitled this revision from [WIP] Use llvm.is_fpclass to implement FP classification functions to Use llvm.is_fpclass to implement FP classification functions.Dec 11 2022, 3:37 AM
sepavloff edited the summary of this revision. (Show Details)
kpn added inline comments.Dec 13 2022, 10:53 AM
clang/test/CodeGen/strictfp_builtins.c
160

Is there a way to test that we're using llvm.is.fpclass() in this case?

sepavloff added inline comments.Dec 13 2022, 11:21 AM
clang/test/CodeGen/strictfp_builtins.c
160

This is a bit different case than binary predicate. isinf_sign returns 0, 1 or -1. It could be implemented using calls of is_fpclass and signbit, but the implementation should be efficient for various targets and for vector data also. It is worth a separate patch.

arsenm requested changes to this revision.Dec 14 2022, 7:03 AM
arsenm added a subscriber: arsenm.

I think the __builtin_isfpclass part should be a separate patch from the rest of the functions. Also needs sema tests for the handling of the immediate argument

clang/include/clang/Basic/Builtins.def
485 ↗(On Diff #481901)

Should use I to mark the isfpclass test mask given you don't try to handle the variable case

clang/lib/CodeGen/CGBuiltin.cpp
3200–3201

Should enforce constant argument with "I" builtin constraint

This revision now requires changes to proceed.Dec 14 2022, 7:03 AM

Remove __builtin_isfpclass

This will produce worse codegen in many situations. I'm working on a stack of patches to convert is.fpclass back to fcmp when legal

sepavloff updated this revision to Diff 485307.Dec 26 2022, 8:31 AM

Update patch

  • Rebase,
  • Add test that checks inlining.
arsenm added inline comments.Jan 6 2023, 5:38 AM
clang/include/clang/Basic/Builtins.def
482–484 ↗(On Diff #485307)

Sorry, I should have clarified, all the new builtins should be added separately from the change of the implementation of the existing builtins

sepavloff updated this revision to Diff 488159.Jan 11 2023, 4:53 AM

Remove support of __builtin_is{subnormal,zero,signaling}, rebased

This change itself LGTM but I think it should wait until after we get more optimizations in to go back to fcmp, and after the release branch

Matt added a subscriber: Matt.Jan 17 2023, 10:57 AM

This change itself LGTM but I think it should wait until after we get more optimizations in to go back to fcmp, and after the release branch

I think these optimizations are mostly in a good state. I still have some outstanding is.fpclass codegen patches (e.g. D143201 and related)

Remove __builtin_isfpclass

I am interested in adding a __builtin_isfpclass, just in a separate patch

arsenm added inline comments.Jun 1 2023, 10:50 AM
clang/lib/CodeGen/CGBuiltin.cpp
2245–2247

Might as well put this in IRBuilder

sepavloff updated this revision to Diff 529156.Jun 6 2023, 10:03 PM

Add createIsFPClass to IRBuilder

Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 10:03 PM
arsenm accepted this revision.Jun 22 2023, 10:37 AM

LGTM

clang/test/CodeGen/isfpclass.c
2

While already covered by existing tests, it might be nicer to test all the builtins in the same file with and without fenv access enabled

This revision is now accepted and ready to land.Jun 22 2023, 10:37 AM

Description now needs updating since this diff no longer adds all the split out pieces

qiucf added a comment.Jul 10 2023, 7:04 PM

Is there any blocker for this to land?

This revision was landed with ongoing or failed builds.Jul 11 2023, 7:35 AM
This revision was automatically updated to reflect the committed changes.