[RISCV][compiler-rt] Update __init_riscv_feature_bits prototype #101472
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Piyou Chen (BeMg) ChangesThis patch add Full diff: https://github.com/llvm/llvm-project/pull/101472.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 0c2ee446aa303..cf233a0fa85c8 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -14300,13 +14300,13 @@ Value *CodeGenFunction::EmitAArch64CpuInit() {
}
Value *CodeGenFunction::EmitRISCVCpuInit() {
- llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
+ llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {VoidPtrTy}, false);
llvm::FunctionCallee Func =
CGM.CreateRuntimeFunction(FTy, "__init_riscv_feature_bits");
auto *CalleeGV = cast<llvm::GlobalValue>(Func.getCallee());
CalleeGV->setDSOLocal(true);
CalleeGV->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
- return Builder.CreateCall(Func);
+ return Builder.CreateCall(Func, {llvm::ConstantPointerNull::get(VoidPtrTy)});
}
Value *CodeGenFunction::EmitX86CpuInit() {
diff --git a/clang/test/CodeGen/builtin-cpu-supports.c b/clang/test/CodeGen/builtin-cpu-supports.c
index 92c407653e660..a0f982a5f6a50 100644
--- a/clang/test/CodeGen/builtin-cpu-supports.c
+++ b/clang/test/CodeGen/builtin-cpu-supports.c
@@ -250,7 +250,7 @@ int test_ppc(int a) {
// CHECK-RV32-NEXT: [[RETVAL:%.*]] = alloca i32, align 4
// CHECK-RV32-NEXT: [[A_ADDR:%.*]] = alloca i32, align 4
// CHECK-RV32-NEXT: store i32 [[A]], ptr [[A_ADDR]], align 4
-// CHECK-RV32-NEXT: call void @__init_riscv_feature_bits()
+// CHECK-RV32-NEXT: call void @__init_riscv_feature_bits(ptr null)
// CHECK-RV32-NEXT: [[TMP0:%.*]] = load i64, ptr getelementptr inbounds ({ i32, [1 x i64] }, ptr @__riscv_feature_bits, i32 0, i32 1, i32 0), align 8
// CHECK-RV32-NEXT: [[TMP1:%.*]] = and i64 [[TMP0]], 1
// CHECK-RV32-NEXT: [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 1
@@ -291,7 +291,7 @@ int test_ppc(int a) {
// CHECK-RV64-NEXT: [[RETVAL:%.*]] = alloca i32, align 4
// CHECK-RV64-NEXT: [[A_ADDR:%.*]] = alloca i32, align 4
// CHECK-RV64-NEXT: store i32 [[A]], ptr [[A_ADDR]], align 4
-// CHECK-RV64-NEXT: call void @__init_riscv_feature_bits()
+// CHECK-RV64-NEXT: call void @__init_riscv_feature_bits(ptr null)
// CHECK-RV64-NEXT: [[TMP0:%.*]] = load i64, ptr getelementptr inbounds ({ i32, [1 x i64] }, ptr @__riscv_feature_bits, i32 0, i32 1, i32 0), align 8
// CHECK-RV64-NEXT: [[TMP1:%.*]] = and i64 [[TMP0]], 1
// CHECK-RV64-NEXT: [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 1
diff --git a/compiler-rt/lib/builtins/cpu_model/riscv.c b/compiler-rt/lib/builtins/cpu_model/riscv.c
index 92931fae64fbf..54b845567d3c2 100644
--- a/compiler-rt/lib/builtins/cpu_model/riscv.c
+++ b/compiler-rt/lib/builtins/cpu_model/riscv.c
@@ -280,14 +280,18 @@ static void initRISCVFeature(struct riscv_hwprobe Hwprobes[]) {
static int FeaturesBitCached = 0;
-void __init_riscv_feature_bits() CONSTRUCTOR_ATTRIBUTE;
+void __init_riscv_feature_bits(void *) CONSTRUCTOR_ATTRIBUTE;
// A constructor function that sets __riscv_feature_bits, and
// __riscv_vendor_feature_bits to the right values. This needs to run
// only once. This constructor is given the highest priority and it should
// run before constructors without the priority set. However, it still runs
// after ifunc initializers and needs to be called explicitly there.
-void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits() {
+
+// PlatformArgs allows the platform to provide pre-computed data and access it
+// without extra effort. For example, Linux could pass the vDSO object to avoid
+// an extra system call.
+void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits(void *PlatformArgs) {
if (FeaturesBitCached)
return;
|
|
@llvm/pr-subscribers-clang-codegen Author: Piyou Chen (BeMg) ChangesThis patch add Full diff: https://github.com/llvm/llvm-project/pull/101472.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 0c2ee446aa303..cf233a0fa85c8 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -14300,13 +14300,13 @@ Value *CodeGenFunction::EmitAArch64CpuInit() {
}
Value *CodeGenFunction::EmitRISCVCpuInit() {
- llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
+ llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {VoidPtrTy}, false);
llvm::FunctionCallee Func =
CGM.CreateRuntimeFunction(FTy, "__init_riscv_feature_bits");
auto *CalleeGV = cast<llvm::GlobalValue>(Func.getCallee());
CalleeGV->setDSOLocal(true);
CalleeGV->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
- return Builder.CreateCall(Func);
+ return Builder.CreateCall(Func, {llvm::ConstantPointerNull::get(VoidPtrTy)});
}
Value *CodeGenFunction::EmitX86CpuInit() {
diff --git a/clang/test/CodeGen/builtin-cpu-supports.c b/clang/test/CodeGen/builtin-cpu-supports.c
index 92c407653e660..a0f982a5f6a50 100644
--- a/clang/test/CodeGen/builtin-cpu-supports.c
+++ b/clang/test/CodeGen/builtin-cpu-supports.c
@@ -250,7 +250,7 @@ int test_ppc(int a) {
// CHECK-RV32-NEXT: [[RETVAL:%.*]] = alloca i32, align 4
// CHECK-RV32-NEXT: [[A_ADDR:%.*]] = alloca i32, align 4
// CHECK-RV32-NEXT: store i32 [[A]], ptr [[A_ADDR]], align 4
-// CHECK-RV32-NEXT: call void @__init_riscv_feature_bits()
+// CHECK-RV32-NEXT: call void @__init_riscv_feature_bits(ptr null)
// CHECK-RV32-NEXT: [[TMP0:%.*]] = load i64, ptr getelementptr inbounds ({ i32, [1 x i64] }, ptr @__riscv_feature_bits, i32 0, i32 1, i32 0), align 8
// CHECK-RV32-NEXT: [[TMP1:%.*]] = and i64 [[TMP0]], 1
// CHECK-RV32-NEXT: [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 1
@@ -291,7 +291,7 @@ int test_ppc(int a) {
// CHECK-RV64-NEXT: [[RETVAL:%.*]] = alloca i32, align 4
// CHECK-RV64-NEXT: [[A_ADDR:%.*]] = alloca i32, align 4
// CHECK-RV64-NEXT: store i32 [[A]], ptr [[A_ADDR]], align 4
-// CHECK-RV64-NEXT: call void @__init_riscv_feature_bits()
+// CHECK-RV64-NEXT: call void @__init_riscv_feature_bits(ptr null)
// CHECK-RV64-NEXT: [[TMP0:%.*]] = load i64, ptr getelementptr inbounds ({ i32, [1 x i64] }, ptr @__riscv_feature_bits, i32 0, i32 1, i32 0), align 8
// CHECK-RV64-NEXT: [[TMP1:%.*]] = and i64 [[TMP0]], 1
// CHECK-RV64-NEXT: [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 1
diff --git a/compiler-rt/lib/builtins/cpu_model/riscv.c b/compiler-rt/lib/builtins/cpu_model/riscv.c
index 92931fae64fbf..54b845567d3c2 100644
--- a/compiler-rt/lib/builtins/cpu_model/riscv.c
+++ b/compiler-rt/lib/builtins/cpu_model/riscv.c
@@ -280,14 +280,18 @@ static void initRISCVFeature(struct riscv_hwprobe Hwprobes[]) {
static int FeaturesBitCached = 0;
-void __init_riscv_feature_bits() CONSTRUCTOR_ATTRIBUTE;
+void __init_riscv_feature_bits(void *) CONSTRUCTOR_ATTRIBUTE;
// A constructor function that sets __riscv_feature_bits, and
// __riscv_vendor_feature_bits to the right values. This needs to run
// only once. This constructor is given the highest priority and it should
// run before constructors without the priority set. However, it still runs
// after ifunc initializers and needs to be called explicitly there.
-void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits() {
+
+// PlatformArgs allows the platform to provide pre-computed data and access it
+// without extra effort. For example, Linux could pass the vDSO object to avoid
+// an extra system call.
+void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits(void *PlatformArgs) {
if (FeaturesBitCached)
return;
|
|
Alongside #101449, this seems to be extending the interface beyond what is described in the current draft spec. Is there some other source of truth I should be looking for? Is this a proposal to change the spec, or an implementation of something that was already decided (at least tentatively)? |
I will soon update these changes in riscv-non-isa/riscv-c-api-doc#74. These changes are based on discussions with @kito-cheng. |
jrtc27
reviewed
Oct 22, 2024
| // PlatformArgs allows the platform to provide pre-computed data and access it | ||
| // without extra effort. For example, Linux could pass the vDSO object to avoid | ||
| // an extra system call. | ||
| void CONSTRUCTOR_ATTRIBUTE __init_riscv_feature_bits(void *PlatformArgs) { |
There was a problem hiding this comment.
If it's a constructor, how can it receive a value? And if it's a constructor, why is Clang also calling it?
There was a problem hiding this comment.
I think clang needs to call it because ifunc resolvers may run before constructors?
But I agree having an argument doesn't make sense.
There was a problem hiding this comment.
I think my point was more "having it be a constructor and called doesn't seem to make a lot of sense", because if the expectation is that every user of the global variable calls the function first then the constructor is pointless, unless it's an "ifunc resolvers must call the function, other users need not" situation? Also, if everyone's calling the function, it could just return the pointer to the data rather than requiring the caller to then get a pointer to the global.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch add
void* PlatformArgsparameter to__init_riscv_feature_bits.PlatformArgsallows the platform to provide pre-computed data and access it without extra effort. For example, Linux could pass the vDSO object to avoid an extra system call.