[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #99958

preames · 2024-07-22T21:58:30Z

Changes included:

Adding CONSTRUCTOR_ATTRIBUTE so that the static data is setup early on in process lifetime. This is required by gcc docs for __builtin_cpu_supports which we hope to implement in terms of this.
Move the length initialization outside of the #if defined(linux) block so that the length field always reflects the size of the structures even if non of the feature bits are non-zero.
Change the __riscv_vendor_feature_bits.length field to match the length of the actual structure.

Note that this change has not been built or tested. I could not figure out how to get a working cross build for compiler-rt setup. @BeMg, if you could confirm this builds and passes tests in your environment, I'd appreciate it.

Changes included: * Adding CONSTRUCTOR_ATTRIBUTE so that the static data is setup early on in process lifetime. This is required by gcc docs for __builtin_cpu_supports which we hope to implement in terms of this. * Move the length initialization outside of the #if defined(__linux__) block so that the length field always reflects the size of the structures even if non of the feature bits are non-zero. * Change the __riscv_vendor_feature_bits.length field to match the length of the actual structure. Note that this change has not been built or tested. I could not figure out how to get a working cross build for compiler-rt setup.

topperc · 2024-07-22T22:21:38Z

compiler-rt/lib/builtins/riscv/feature_bits.c

-void __init_riscv_feature_bits() {
+void __init_riscv_feature_bits() CONSTRUCTOR_ATTRIBUTE;
+
+// A constructor function that is sets __riscv_feature_bits, and


"is sets" -> "sets"?

BeMg · 2024-07-23T02:32:19Z

Encounter some syntax error during the compiler-rt building.

/scratch/pchen1/LLVM_UPSTREAM/LLVM/linux_install/bin/clang --target=riscv64-unknown-linux-gnu -DVISIBILITY_HIDDEN  -Wall -Wno-unused-parameter -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=suspicious-memaccess -Werror=builtin-memcpy-chk-size -Werror=array-bounds-pointer-arithmetic -Werror=return-stack-address -Werror=sizeof-array-decay -Werror=format-insufficient-args -Wformat -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -MD -MT lib/builtins/CMakeFiles/clang_rt.builtins-riscv64.dir/riscv/feature_bits.c.o -MF lib/builtins/CMakeFiles/clang_rt.builtins-riscv64.dir/riscv/feature_bits.c.o.d -o lib/builtins/CMakeFiles/clang_rt.builtins-riscv64.dir/riscv/feature_bits.c.o -c /scratch/pchen1/LLVM_UPSTREAM/LLVM/llvm-project/compiler-rt/lib/builtins/riscv/feature_bits.c
/scratch/pchen1/LLVM_UPSTREAM/LLVM/llvm-project/compiler-rt/lib/builtins/riscv/feature_bits.c:278:34: error: expected function body after function declarator
  278 | void __init_riscv_feature_bits() CONSTRUCTOR_ATTRIBUTE;
      |                                  ^
/scratch/pchen1/LLVM_UPSTREAM/LLVM/llvm-project/compiler-rt/lib/builtins/riscv/feature_bits.c:285:6: error: variable has incomplete type 'void'
  285 | void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits() {
      |      ^
/scratch/pchen1/LLVM_UPSTREAM/LLVM/llvm-project/compiler-rt/lib/builtins/riscv/feature_bits.c:285:27: error: expected ';' after top level declarator
  285 | void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits() {
      |                           ^
      |                           ;
3 errors generated.

BeMg · 2024-07-23T02:55:25Z

compiler-rt/lib/builtins/riscv/feature_bits.c

@@ -6,6 +6,8 @@
 //
 //===----------------------------------------------------------------------===//

+#include "cpu_model.h"


Here we have two solution to avoid the build fail

Move feature_bits.c from compiler-rt/lib/builtins/riscv/feature_bits.c to compiler-rt/lib/builtins/cpu_model/riscv.c, and update the compiler-rt/lib/builtins/CMakeLists.txt. Then cpu_model.h can be searched correctly. I create a commit for this case BeMg@0aff090

Simply copy code section from cpu_model.h to here

#if __has_attribute(constructor) #if __GNUC__ >= 9 // Ordinarily init priorities below 101 are disallowed as they are reserved for // the implementation. However, we are the implementation, so silence the // diagnostic, since it doesn't apply to us. #pragma GCC diagnostic ignored "-Wprio-ctor-dtor" #endif // We're choosing init priority 90 to force our constructors to run before any // constructors in the end user application (starting at priority 101). This // value matches the libgcc choice for the same functions. #define CONSTRUCTOR_ATTRIBUTE __attribute__((constructor(90))) #else // FIXME: For MSVC, we should make a function pointer global in .CRT$X?? so that // this runs during initialization. #define CONSTRUCTOR_ATTRIBUTE #endif

+1 for option 1

I also prefer option 1. @BeMg Can you take over this review? I don't have a workable build environment for compiler-rt at the moment. If you're happy with this change, feel free to take it, apply the change needed for it to build (option 1) and land.

I'm in the process of doing a native build of the 19.x release branch (including compiler-rt), but that's likely going to be another ~36 hours or so before it's complete and I can use it for incremental patch testing.

p.s. I applied the diff from your change and pushed it to this review in case that makes anything easier.

@BeMg

This implements the __builtin_cpu_init and __builtin_cpu_supports builtin routines based on the compiler runtime changes in #85790. This is inspired by #85786. Major changes are a) a restriction in scope to only the builtins (which have a much narrower user interface), and the avoidance of false generality. This change deliberately only handles group 0 extensions (which happen to be all defined ones today), and avoids the tblgen changes from that review. I don't have an environment in which I can actually test this, but @BeMg has been kind enough to report that this appears to work as expected. Before this can make it into a release, we need a change such as #99958. The gcc docs claim that cpu_support can be called by "normal" code without calling the cpu_init routine because the init routine will have been called by a high priority constructor. Our current compiler-rt mechanism does not do this.

@BeMg

Patch by @BeMg

BeMg · 2024-07-23T17:29:21Z

I think the compiler-rt building is fine now. But a little concern on behavior description on another patch.

Before this can make it into a release, we need a change such as #99958. The gcc docs claim that cpu_support can be called by "normal" code without calling the cpu_init routine because the init routine will have been called by a high priority constructor. Our current compiler-rt mechanism does not do this.

Does it mean we could use the __builtin_cpu_supports without call __builtin_cpu_init explicitly?

In my local test, if I didn't invoke __builtin_cpu_init explicitly, every __builtin_cpu_supports return false even with CONSTRUCTOR_ATTRIBUTE.

Maybe we missing something, or I misunderstand the behavior.

preames · 2024-07-23T20:05:28Z

I think the compiler-rt building is fine now. But a little concern on behavior description on another patch.

Before this can make it into a release, we need a change such as #99958. The gcc docs claim that cpu_support can be called by "normal" code without calling the cpu_init routine because the init routine will have been called by a high priority constructor. Our current compiler-rt mechanism does not do this.

Does it mean we could use the __builtin_cpu_supports without call __builtin_cpu_init explicitly?

Yes, it does.

In my local test, if I didn't invoke __builtin_cpu_init explicitly, every __builtin_cpu_supports return false even with CONSTRUCTOR_ATTRIBUTE.

And this is just normal code? Not like an ifunc resolver or something? That's very surprising.

Maybe we missing something, or I misunderstand the behavior.

Sounds like we're missing something. I have no idea what though. To start debugging:

Which compiler are you using? What linker?

preames · 2024-07-23T21:11:38Z

I've written some simple test cases for the attribute((constructor)) feature, and everything appears to work as I'd expect. I didn't find any super obvious breakage.

BeMg · 2024-07-23T23:50:58Z

I've written some simple test cases for the attribute((constructor)) feature, and everything appears to work as I'd expect. I didn't find any super obvious breakage.

After rebuilding the test environment, it now works as expected. I believe the issue was due to mixing with an old version of the compiler-rt archive file. I apologize for the disturbance. I'm going to merge it on #100158.

Changes included: - Adding CONSTRUCTOR_ATTRIBUTE so that the static data is setup early on in process lifetime. This is required by gcc docs for __builtin_cpu_supports which we hope to implement in terms of this. - Move the length initialization outside of the #if defined(linux) block so that the length field always reflects the size of the structures even if non of the feature bits are non-zero. - Change the __riscv_vendor_feature_bits.length field to match the length of the actual structure. Note: Copy from #99958 --------- Co-authored-by: Philip Reames <[email protected]>

preames · 2024-07-24T01:05:22Z

After rebuilding the test environment, it now works as expected. I believe the issue was due to mixing with an old version of the compiler-rt archive file. I apologize for the disturbance. I'm going to merge it on #100158.

No problem, that's pretty much best outcome here. Thanks!

preames · 2024-07-24T01:05:47Z

Landed as #100158

@BeMg

This implements the __builtin_cpu_init and __builtin_cpu_supports builtin routines based on the compiler runtime changes in #85790. This is inspired by #85786. Major changes are a) a restriction in scope to only the builtins (which have a much narrower user interface), and the avoidance of false generality. This change deliberately only handles group 0 extensions (which happen to be all defined ones today), and avoids the tblgen changes from that review. I don't have an environment in which I can actually test this, but @BeMg has been kind enough to report that this appears to work as expected. Before this can make it into a release, we need a change such as #99958. The gcc docs claim that cpu_support can be called by "normal" code without calling the cpu_init routine because the init routine will have been called by a high priority constructor. Our current compiler-rt mechanism does not do this.

Summary: Changes included: - Adding CONSTRUCTOR_ATTRIBUTE so that the static data is setup early on in process lifetime. This is required by gcc docs for __builtin_cpu_supports which we hope to implement in terms of this. - Move the length initialization outside of the #if defined(linux) block so that the length field always reflects the size of the structures even if non of the feature bits are non-zero. - Change the __riscv_vendor_feature_bits.length field to match the length of the actual structure. Note: Copy from #99958 --------- Co-authored-by: Philip Reames <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250767

preames requested review from kito-cheng, BeMg and topperc July 22, 2024 21:58

llvmbot added compiler-rt compiler-rt:builtins labels Jul 22, 2024

topperc reviewed Jul 22, 2024

View reviewed changes

preames added 2 commits July 22, 2024 19:32

Address review comment

9029205

Attempt to fix build

4d589a3

BeMg reviewed Jul 23, 2024

View reviewed changes

preames mentioned this pull request Jul 23, 2024

[RISCV] Support __builtin_cpu_init and __builtin_cpu_supports #99700

Merged

Move feature_bits.c into cpu_model

ede49c7

Patch by @BeMg

BeMg mentioned this pull request Jul 23, 2024

[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #100158

Merged

preames closed this Jul 24, 2024

preames deleted the pr-compiler-rt-features-bits-fix branch July 24, 2024 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #99958

[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #99958

Uh oh!

preames commented Jul 22, 2024

Uh oh!

topperc Jul 22, 2024

Uh oh!

BeMg commented Jul 23, 2024 •

edited

Loading

Uh oh!

BeMg Jul 23, 2024

Uh oh!

kito-cheng Jul 23, 2024

Uh oh!

preames Jul 23, 2024 •

edited

Loading

Uh oh!

BeMg commented Jul 23, 2024

Uh oh!

preames commented Jul 23, 2024

Uh oh!

preames commented Jul 23, 2024

Uh oh!

BeMg commented Jul 23, 2024

Uh oh!

preames commented Jul 24, 2024

Uh oh!

preames commented Jul 24, 2024

Uh oh!

Uh oh!

[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #99958

[RISCV][compiler-rt] Small fixes for __riscv_feature_bits #99958

Uh oh!

Conversation

preames commented Jul 22, 2024

Uh oh!

topperc Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

BeMg commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BeMg Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

kito-cheng Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

preames Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeMg commented Jul 23, 2024

Uh oh!

preames commented Jul 23, 2024

Uh oh!

preames commented Jul 23, 2024

Uh oh!

BeMg commented Jul 23, 2024

Uh oh!

preames commented Jul 24, 2024

Uh oh!

preames commented Jul 24, 2024

Uh oh!

Uh oh!

BeMg commented Jul 23, 2024 •

edited

Loading

preames Jul 23, 2024 •

edited

Loading