Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[builtins] Refactor cpu_model support to reduce #if nesting. NFCI #75635

Merged
merged 7 commits into from
Dec 19, 2023
Merged

[builtins] Refactor cpu_model support to reduce #if nesting. NFCI #75635

merged 7 commits into from
Dec 19, 2023

Conversation

jroelofs
Copy link
Contributor

@jroelofs jroelofs commented Dec 15, 2023 •
edited
Loading

No description provided.

@jroelofs
[𝘀𝗽𝗿] initial version
336d162 
Created using spr 1.3.4
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 15, 2023 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@jroelofs jroelofs mentioned this pull request Dec 15, 2023
Merged
@jroelofs
fix formatting, address my own nits
bf23095 
Created using spr 1.3.4
mstorsjo
mstorsjo reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks reasonable overall, but there's a typo in the name of Fuchsia, which I believe would lead to its codepaths being entirely missed :-)

Thanks, in this form I think the ifdef jungle is much more manageable.

compiler-rt/lib/builtins/cpu_model/aarch64.c Outdated
#if defined(__FreeBSD__)
#include "aarch64/fmv/freebsd.inc"
#include "aarch64/fmv/mrs.inc"
#elif defined(__Fucsia__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you've misspelled Fuchsia consistently :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ thanks

@jroelofs
s/Fucsia/Fuchsia/
ce4e19c 
Created using spr 1.3.4
@jroelofs
reorder includes
4919e63 
Created using spr 1.3.4
@jroelofs
turn off clang-format, it's wrong
6f3d997 
Created using spr 1.3.4
ilinpv
ilinpv reviewed Dec 18, 2023
View reviewed changes
Copy link
Contributor

@ilinpv ilinpv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for refactoring! It indeed becomes necessary and looks well-structured now, small nit - rename lse_atomics/fucsia.inc and fmv/fucsia.inc files as well.

@jroelofs
s/fucsia/fuchsia/
6699a29 
Created using spr 1.3.4
@jroelofs
add EOF newline
4d7f76a 
Created using spr 1.3.4
@jroelofs jroelofs merged commit 025d048 into main Dec 19, 2023
4 checks passed
@jroelofs jroelofs deleted the users/jroelofs/spr/builtins-refactor-cpu_model-support-to-reduce-if-nesting-nfci branch December 19, 2023 18:09
jroelofs added a commit that referenced this pull request Dec 19, 2023
@jroelofs
[builtins] Refactor cpu_model support to reduce #if nesting. NFCI
9237cfa 
Reviewers: petrhosek, DavidSpickett

Pull Request: #75635
@vitalybuka
Copy link
Collaborator

Breaks https://lab.llvm.org/buildbot/#/builders/269/builds/2975

@jroelofs
Copy link
Contributor Author

There were a few follow-ups to address that. Sorry.

@jroelofs
Copy link
Contributor Author

@jroelofs
Copy link
Contributor Author

@petrhosek I don't have a Fucshia sysroot to build this against, so I think it would help me a lot if you could grab the preprocessed version of the cpu_model.c before all these changes and stick it in a github gist... I feel a little bad for dragging this out in-tree.

These #if __has_include(...) conditionals would evaluate to false on Fuchsia since we don't have those headers, but they have been moved in the refactored version.

In that case, if I'm reading it correctly, all of the #if defined(__Fuchsia__) support for both initializing the LSE atomics detection bool, and FMV in cpu_model.c are dead code, and we can drop those entries in the new aarch64.c.

@petrhosek petrhosek mentioned this pull request Dec 23, 2023
Merged
petrhosek added a commit to petrhosek/llvm-project that referenced this pull request Dec 23, 2023
@petrhosek
[builtins] Fix CPU feature detection for Zircon
b2868be 
This is a follow up to llvm#75635 which broke the build on Fuchsia. We don't
support ifunc on Fuchsia so we shouldn't define __init_cpu_features. For
__init_cpu_features_resolver we have to use _zx_system_get_features as a
Zircon native solution.
petrhosek added a commit that referenced this pull request Dec 24, 2023
@petrhosek
[builtins] Fix CPU feature detection for Zircon (#76276)
09e6f12 
This is a follow up to #75635 which broke the build on Fuchsia. We don't
support ifunc on Fuchsia so we shouldn't define __init_cpu_features. For
__init_cpu_features_resolver we have to use _zx_system_get_features as a
Zircon native solution.
DimitryAndric added a commit that referenced this pull request Dec 28, 2023
@DimitryAndric
[builtins] Fix CPU feature detection for FreeBSD on AArch64
ef7cd10 
This is a follow-up to #75635 which broke the build for FreeBSD on
AArch64:

```
compiler-rt/lib/builtins/cpu_model/aarch64/lse_atomics/freebsd.inc:3:16: error: call to undeclared function 'elf_aux_info'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3 |   int result = elf_aux_info(AT_HWCAP, &hwcap, sizeof hwcap);
      |                ^
```

Using `elf_aux_info()` requires including `<sys/auxv.h>` first. To
prevent redeclaration issues with `hwcap.inc` attempting to define
`HWCAP_xxx` macros before `<sys/auxv.h>` does so, include `<sys/auxv.h>`
before any of the `.inc` files on FreeBSD.
@DimitryAndric DimitryAndric mentioned this pull request Dec 28, 2023
Merged
DimitryAndric added a commit that referenced this pull request Dec 29, 2023
@DimitryAndric
[builtins] Fix CPU feature detection for FreeBSD on AArch64 (#76532)
953ae94 
 [builtins] Fix CPU feature detection for FreeBSD on AArch64

This is a follow-up to #75635 which broke the build for FreeBSD on
AArch64:

```
compiler-rt/lib/builtins/cpu_model/aarch64/lse_atomics/freebsd.inc:3:16: error: call to undeclared function 'elf_aux_info'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3 |   int result = elf_aux_info(AT_HWCAP, &hwcap, sizeof hwcap);
      |                ^
```

Using `elf_aux_info()` requires including `<sys/auxv.h>` first. To
prevent redeclaration issues with `hwcap.inc` attempting to define
`HWCAP_xxx` macros before `<sys/auxv.h>` does so, include `<sys/auxv.h>`
before any of the `.inc` files on FreeBSD.
jroelofs added a commit to swiftlang/llvm-project that referenced this pull request Jan 10, 2024
@jroelofs
Refactor cpu_model support to reduce #if nesting. NFCI
1d13258 
Reviewers: petrhosek, DavidSpickett

Pull Request: llvm#75635
jroelofs added a commit to swiftlang/llvm-project that referenced this pull request Jan 10, 2024
@jroelofs
Refactor cpu_model support to reduce #if nesting. NFCI
5098ff8 
Reviewers: petrhosek, DavidSpickett

Pull Request: llvm#75635
@ilinpv ilinpv mentioned this pull request Feb 20, 2024
Merged
ilinpv added a commit to ilinpv/llvm-project that referenced this pull request Feb 20, 2024
@ilinpv
[AArch64] Implement __builtin_cpu_supports, compiler-rt tests.
7ada935 
The patch complements llvm#68919
and adds AArch64 support for builtin
__builtin_cpu_supports("feature1+...+featureN")
which return true if all specified CPU features in argument are
detected. Also compiler-rt aarch64 native run tests for features
detection mechanism were added and 'cpu_model' check was fixed after its
refactor merged llvm#75635
Original RFC was https://reviews.llvm.org/D153153
ilinpv added a commit to ilinpv/llvm-project that referenced this pull request Feb 22, 2024
@ilinpv
[AArch64] Implement __builtin_cpu_supports, compiler-rt tests.
7283a6f 
The patch complements llvm#68919
and adds AArch64 support for builtin
__builtin_cpu_supports("feature1+...+featureN")
which return true if all specified CPU features in argument are
detected. Also compiler-rt aarch64 native run tests for features
detection mechanism were added and 'cpu_model' check was fixed after its
refactor merged llvm#75635
Original RFC was https://reviews.llvm.org/D153153
ilinpv added a commit that referenced this pull request Feb 22, 2024
@ilinpv
[AArch64] Implement __builtin_cpu_supports, compiler-rt tests. (#82378)
568baba 
The patch complements #68919
and adds AArch64 support for builtin
`__builtin_cpu_supports("feature1+...+featureN")`
which return true if all specified CPU features in argument are
detected. Also compiler-rt aarch64 native run tests for features
detection mechanism were added and 'cpu_model' check was fixed after its
refactor merged #75635 Original
RFC was https://reviews.llvm.org/D153153
qihangkong pushed a commit to rvgpu/rvgpu-llvm that referenced this pull request Apr 23, 2024
@jroelofs @wangxiang090105
[builtins] Refactor cpu_model support to reduce #if nesting. NFCI
d48758d 
Reviewers: petrhosek, DavidSpickett

Pull Request: llvm/llvm-project#75635
qihangkong pushed a commit to rvgpu/rvgpu-llvm that referenced this pull request Apr 23, 2024
@jroelofs @wangxiang090105
[builtins] Refactor cpu_model support to reduce #if nesting. NFCI
5807aab 
Reviewers: petrhosek, DavidSpickett

Pull Request: llvm/llvm-project#75635
qihangkong pushed a commit to rvgpu/rvgpu-llvm that referenced this pull request Apr 23, 2024
@jroelofs @wangxiang090105
fixup! [builtins] Refactor cpu_model support to reduce #if nesting. NFCI
c1b692e 
llvm/llvm-project#75635 (comment)

```
/b/s/w/ir/x/w/llvm_build/./bin/clang --target=aarch64-unknown-linux-gnu --sysroot=/b/s/w/ir/x/w/cipd/linux -DHAS_ASM_LSE -DVISIBILITY_HIDDEN  --target=aarch64-unknown-linux-gnu -O2 -g -DNDEBUG -fno-lto -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -DCOMPILER_RT_HAS_FLOAT16 -MD -MT CMakeFiles/clang_rt.builtins-aarch64.dir/cpu_model/aarch64.c.o -MF CMakeFiles/clang_rt.builtins-aarch64.dir/cpu_model/aarch64.c.o.d -o CMakeFiles/clang_rt.builtins-aarch64.dir/cpu_model/aarch64.c.o -c /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/builtins/cpu_model/aarch64.c
In file included from /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/builtins/cpu_model/aarch64.c:43:
/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/builtins/cpu_model/aarch64/lse_atomics/sysauxv.inc:5:41: error: use of undeclared identifier 'HWCAP_ATOMICS'
    5 |   __aarch64_have_lse_atomics = (hwcap & HWCAP_ATOMICS) != 0;
      |                                         ^
1 error generated.
```
qihangkong pushed a commit to rvgpu/rvgpu-llvm that referenced this pull request Apr 23, 2024
@jroelofs @wangxiang090105
fixup! [builtins] Refactor cpu_model support to reduce #if nesting. NFCI
bed19ef 
llvm/llvm-project#75635 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstorsjo mstorsjo mstorsjo left review comments

@ilinpv ilinpv ilinpv left review comments

@petrhosek petrhosek Awaiting requested review from petrhosek

@DavidSpickett DavidSpickett Awaiting requested review from DavidSpickett

Assignees
No one assigned
Labels
compiler-rt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jroelofs @vitalybuka @petrhosek @aeubanks @mstorsjo @ilinpv @llvmbot