Use LLVM to detect CPU features by default if --features aren't specified. #1580
Conversation
jemc
added
the
changelog - fixed
|
@dipinhora - please confirm that this fix is what you had in mind. |
|
@jemc This is roughly what I had in mind with some minor tweaks (which I'll mention directly on the diff via the review comments). |
src/libponyc/codegen/codegen.c
Outdated
| @@ -882,7 +882,7 @@ bool codegen_init(pass_opt_t* opt) | |||
| if(opt->features != NULL) | |||
| opt->features = LLVMCreateMessage(opt->features); | |||
| else | |||
| opt->features = LLVMCreateMessage(""); | |||
| opt->features = LLVMGetHostCPUFeatures(); | |||
There was a problem hiding this comment.
This would break cross compiling since it would always set CPU features to those of the host if no CPU features were specified.
I'd suggest instead the following logic:
if(opt->features != NULL)
opt->features = LLVMCreateMessage(opt->features);
else
if((opt->cpu == LLVMGetHostCPUName()) && (opt->triple == default_triple_ifdef))
opt->features = LLVMGetHostCPUFeatures();
else
opt->features = LLVMCreateMessage("");I think this would ensure that the host CPU features would only be used only for a native compile for the current host and not if someone was cross compiling.
There was a problem hiding this comment.
Sounds good to me - updated and squashed into 1706833.
There was a problem hiding this comment.
@jemc I apologize that I wasn't clear. The logic I suggested was just high level pseudo-code and wasn't actual functioning C code. I should have been more clear about the fact that it was only pseudo code.
I can get you the correct C code but that will take me some time to get right.
There was a problem hiding this comment.
@jemc The following diff works in my limited testing on linux. I had to change how the assertion was being done or else the CPU features weren't properly detected on a config=release build (according to my temporary printf validations).
diff --git a/src/libponyc/codegen/codegen.c b/src/libponyc/codegen/codegen.c
index bbadb25..f39239e 100644
--- a/src/libponyc/codegen/codegen.c
+++ b/src/libponyc/codegen/codegen.c
@@ -882,12 +882,24 @@ bool codegen_init(pass_opt_t* opt)
if(opt->features != NULL)
opt->features = LLVMCreateMessage(opt->features);
else
- if((opt->cpu == LLVMGetHostCPUName()) && \
- (opt->triple == default_triple_ifdef))
+ {
+ char* host_cpu = LLVMGetHostCPUName();
+#ifdef PLATFORM_IS_MACOSX
+ // This is to prevent XCode 7+ from claiming OSX 14.5 is required.
+ char* default_triple = LLVMCreateMessage("x86_64-apple-macosx");
+#else
+ char* default_triple = LLVMGetDefaultTargetTriple();
+#endif
+ if((strcmp(opt->cpu, host_cpu) == 0) && \
+ (strcmp(opt->triple, default_triple) == 0))
opt->features = LLVMGetHostCPUFeatures();
else
opt->features = LLVMCreateMessage("");
+ free(host_cpu);
+ free(default_triple);
+ }
+
return true;
}
diff --git a/src/libponyc/codegen/host.cc b/src/libponyc/codegen/host.cc
index bea4196..417a4a4 100644
--- a/src/libponyc/codegen/host.cc
+++ b/src/libponyc/codegen/host.cc
@@ -36,7 +36,10 @@ char* LLVMGetHostCPUName()
char* LLVMGetHostCPUFeatures()
{
StringMap<bool> features;
- assert(sys::getHostCPUFeatures(features));
+ bool got_features = sys::getHostCPUFeatures(features);
+
+ assert(got_features);
+ (void)got_features;
// Calculate the size of buffer that will be needed to return all features.
size_t buf_size = 0;
jemc
force-pushed
the
feature/host-cpu-features
branch
from
ca17f03 to
1706833
Compare
|
I think there is an open issue from ages ago for recording what llvm features were used to compile ponyc so it could be part of debug output. Would this change in any way make that feature more possible than before? |
|
@SeanTAllen I'm not sure I understand that issue based on your description. |
|
Sorry yes I meant programs compiled by ponyc, not ponyc itself |
|
@SeanTAllen Is #759 the issue you're referring to? If yes, then, yes, we can use the changes in this PR to implement at least part (that issue doesn't actually define what information is in scope/out of scope) of that feature for native builds without much additional effort. Cross compiling makes things more complex and wouldn't be supported for that feature without additional work (not sure how much more). Also, this is with the assumption that this is |
jemc
force-pushed
the
feature/host-cpu-features
branch
from
1706833 to
dbbae89
Compare
|
@dipinhora - Reworked the logic again in dbbae89, to avoid a missing identifier error and some memory leaks (from the LLVMCreateMessage calls). Please take another look. |
|
@jemc It seems you didn't see the diff I put in the review comments. I blame github review's commenting weirdness. 8*/ Please take a look, as it is an implementation of my original suggestion in working code. Sorry again for not being clear earlier that my suggestion was just pseudo-code and not actual working code. Your solution also looks good. But you do need the change in the following diff or else diff --git a/src/libponyc/codegen/host.cc b/src/libponyc/codegen/host.cc
index bea4196..417a4a4 100644
--- a/src/libponyc/codegen/host.cc
+++ b/src/libponyc/codegen/host.cc
@@ -36,7 +36,10 @@ char* LLVMGetHostCPUName()
char* LLVMGetHostCPUFeatures()
{
StringMap<bool> features;
- assert(sys::getHostCPUFeatures(features));
+ bool got_features = sys::getHostCPUFeatures(features);
+
+ assert(got_features);
+ (void)got_features;
// Calculate the size of buffer that will be needed to return all features.
size_t buf_size = 0; |
jemc
force-pushed
the
feature/host-cpu-features
branch
from
dbbae89 to
0336ea0
Compare
|
@dipinhora Thanks - applied and squashed into 0336ea0. |
This PR fixes #1176 using the proposed approach discussed in that ticket.
Specifically, if the user does not specify specific
--featuresin the invocation, we use LLVM'sgetHostCPUFeaturesfunction to get the list of features (and format them into the expected features string format).