This is an archive of the discontinued LLVM Phabricator instance.

Add __builtin_set_flt_rounds
ClosedPublic

Authored by xiongji90 on Mar 9 2023, 9:34 PM.

Details

Summary

This builtin will be converted to llvm.set.rounding intrinsic in IR level and should be work with "#pragma STDC FENV_ACCESS ON" since it changes default FP environment. Users can change rounding mode via this builtin without introducing libc dependency.
This patch re-lands https://reviews.llvm.org/rG24b823554acd25009731b2519880aa18c7263550 , previous patch fails due to a test case bug. The builtin "__builtin_set_flt_rounds" are restricted to work only on x86, x86_64, arm target but previous test is in builtin.c which will run on other targets such ppc64, so it breaks other targets' build. The new patch moves the test to a separate file and use "-triple x86_64-gnu-linux", "-triple x86_64-windows-msvc", "-triple aarch64-gnu-linux", "-triple aarch64-windows-msvc" to run the lit test

Diff Detail

Event Timeline

xiongji90 created this revision.Mar 9 2023, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 9:34 PM
xiongji90 requested review of this revision.Mar 9 2023, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 9:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Hi, @andrew.w.kaylor @rjmccall @sepavloff @aaron.ballman
This patch re-lands previous patch to add builtin_set_flt_rounds, previous patch broke PPC64 buildbot due to test case bug. Previously, tests for builtin_set_flt_rounds was located in CodeGen/builtin.c which would run on all triples but this builtin was restricted to work only on Intel and Arm platform, so the builtin.c will fail on targets such as PPC64. I move the test into a separate file and use following commands to run the lit test:
RUN: %clang_cc1 -triple x86_64-gnu-linux %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple x86_64-windows-msvc %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple aarch64-gnu-linux %s -emit-llvm -o - | FileCheck %s
RUN: %clang_cc1 -triple aarch64-windows-msvc %s -emit-llvm -o - | FileCheck %s
Could you help review again?

Thank you for this! You should also add a release note and some documentation for the new builtin.

clang/test/CodeGen/builtin_set_flt_rounds.c
1–4

Minor cleanup.

5–7

I think you should also add some Sema tests for correct and misuse of the builtin:

__builtin_set_flt_rounds(1); // OK

struct S { int a; } s;
__builtin_set_flt_rounds(s); // This should diagnose, right?

__builtin_set_flt_rounds(1.0f); // Should this implicitly convert or is this an error?

and that sema test can additionally test what happens when you call the builtin on an unsupported architecture.

xiongji90 updated this revision to Diff 504504.Mar 12 2023, 8:54 PM

Add semachecking test for unsupported platform.

xiongji90 marked an inline comment as done.Mar 12 2023, 10:00 PM
xiongji90 added inline comments.
clang/test/CodeGen/builtin_set_flt_rounds.c
5–7

Hi, @aaron.ballman
I added a new test in test/Sema to check the builtin on platforms unsupported.
Thanks very much.

Add SemaChecking test for incompatible input arguments for __builtin_set_flt_rounds.

Thanks for the additional tests! Can you also add a release note to clang/docs/ReleaseNotes.rst and document the new builtin in clang\docs\LanguageExtensions.rst?

clang/test/Sema/builtin_set_flt_rounds.c
1–11 ↗(On Diff #504508)

Simplify the test somewhat.

xiongji90 marked an inline comment as done.

Simplify test and update release doc

Thanks for the additional tests! Can you also add a release note to clang/docs/ReleaseNotes.rst and document the new builtin in clang\docs\LanguageExtensions.rst?

Hi, @aaron.ballman
I have simplified the test as you suggested and I also updated release doc to add "builtin_set_flt_rounds" in fp support section. To LanguageExtension.rst, I prefer to creating a patch since we already had another builtin "builtin_flt_rounds" which was not documented, I plan to document these 2 builtin together.
Thanks very much.

aaron.ballman accepted this revision.Mar 14 2023, 6:55 AM

I have simplified the test as you suggested and I also updated release doc to add "builtin_set_flt_rounds" in fp support section. To LanguageExtension.rst, I prefer to creating a patch since we already had another builtin "builtin_flt_rounds" which was not documented, I plan to document these 2 builtin together.

Documenting the two builtins together in a follow-up patch sounds fine to me, thank you!

LGTM

This revision is now accepted and ready to land.Mar 14 2023, 6:55 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 7:57 PM
This revision was automatically updated to reflect the committed changes.
xiongji90 marked an inline comment as not done.