This is an archive of the discontinued LLVM Phabricator instance.

clang: Add __builtin_elementwise_rint and nearbyint
ClosedPublic

Authored by arsenm on Jun 18 2023, 5:20 PM.

Details

Summary

These are basically the same thing and only differ for strictfp,
so add both for future proofing. Note all the elementwise functions are
currently broken for strictfp, and use non-constrained ops. Add a test
that demonstrates this, but doesn't attempt to fix it.

Diff Detail

Event Timeline

arsenm created this revision.Jun 18 2023, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2023, 5:20 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
arsenm requested review of this revision.Jun 18 2023, 5:20 PM
sepavloff added inline comments.Jun 18 2023, 10:09 PM
clang/docs/LanguageExtensions.rst
656

"halfway'" is superfluous here.

657

nearbyint should not rise only inexact exception. Other exceptions, like invalid are allowed.

662

"halfway'" is superfluous here.

clang/test/CodeGen/strictfp-elementwise-bulitins.cpp
2

For X86 -fexperimental-strict-floating-point is not needed.

13–14

Why vector type is bitcasted to scalar? The function must return <2 x float>, no?

arsenm updated this revision to Diff 532590.Jun 19 2023, 3:50 AM
arsenm marked 3 inline comments as done.
arsenm added inline comments.
clang/test/CodeGen/strictfp-elementwise-bulitins.cpp
13–14

I assume this is some ABI thing. It doesn't happen for other targets and the actual code is correct. I've avoided it by using float4 instead

bob80905 added inline comments.Jun 19 2023, 11:22 AM
clang/docs/LanguageExtensions.rst
656

typo with the word current.

clang/docs/ReleaseNotes.rst
234–239

Don't think this for is necessary or intended, and it appears in the nearbyint description too.

arsenm updated this revision to Diff 533007.Jun 20 2023, 11:53 AM
arsenm marked an inline comment as done.

Keep fixing documentation

arsenm marked an inline comment as done.Jun 20 2023, 11:53 AM
bob80905 added inline comments.Jun 20 2023, 1:59 PM
clang/docs/LanguageExtensions.rst
656

Nit: I don't think a "current rounding direction" exists, and personally don't see the significance of the second half of this sentence.

clang/lib/Sema/SemaChecking.cpp
18583

I don't believe you intended to remove all this code in your latest update, did you?

arsenm added inline comments.Jun 20 2023, 3:33 PM
clang/docs/LanguageExtensions.rst
656

It does exist, we just happen to ignore floating point environment most of the time. The current state of things is we have 3 LLVM intrinsics with the same behavior (nearbyint, rint and roundeven). The distinction only exists with fenv access / strictfp

clang/lib/Sema/SemaChecking.cpp
18583

I didn't delete any code?

arsenm added inline comments.Jun 22 2023, 9:47 AM
clang/lib/Sema/SemaChecking.cpp
18583

Are you looking at a history diff? I see nothing deleted here

bob80905 added inline comments.Jun 22 2023, 10:22 AM
clang/lib/Sema/SemaChecking.cpp
18583

I am unsure what I was seeing prior, but I no longer see any deleted code, you can count this comment as resolved.

bob80905 added inline comments.Jun 22 2023, 10:24 AM
clang/test/CodeGen/strictfp-elementwise-bulitins.cpp
191

I noticed a missing pattern here. Did you mean to write canonicalize instead of TRUNC?

I'd like to ask, is there a reason why there isn't a test for these new builtins under SemaCXX/builtins_elementwise_math.cpp?

arsenm added inline comments.Jun 22 2023, 11:27 AM
clang/test/CodeGen/strictfp-elementwise-bulitins.cpp
191

This is just the generated check from the IR names. It doesn't matter, but there's a pre-existing "bug" where the debug value name is trunc for canonicalize.

arsenm updated this revision to Diff 533779.Jun 22 2023, 2:08 PM

Rebase on fix for wrong debug name

arsenm updated this revision to Diff 533789.Jun 22 2023, 2:46 PM

SemaCXX test

This comment was removed by bob80905.

Should there be tests added for usage of scalable vector types for RISC-V / AArch64?
I typically have added such tests in the past, as shown here:
https://reviews.llvm.org/D135011

Also, would you be able to add something at the top of the strictfp file that states the purpose? Like "testing that we don't use the constrained intrinsics"?

Should there be tests added for usage of scalable vector types for RISC-V / AArch64?
I typically have added such tests in the past, as shown here:
https://reviews.llvm.org/D135011

Test coverage seems to be spotty for them. I think a separate patch to get consistent SVE testing of this should be separate

Also, would you be able to add something at the top of the strictfp file that states the purpose? Like "testing that we don't use the constrained intrinsics"?

Done

arsenm updated this revision to Diff 534111.Jun 23 2023, 4:01 PM

Add fixme to test

This revision is now accepted and ready to land.Jun 23 2023, 4:15 PM