Add an option to add CPUID tag to sysimg and .ji files #19486
Conversation
| JL_DLLEXPORT uint64_t jl_cpuid_tag(void) { | ||
| uint32_t info[4]; | ||
| jl_cpuid((int32_t*)info, 1); | ||
| return (((uint64_t)info[2])|(((uint64_t)info[3])<<32)); |
There was a problem hiding this comment.
please add spaces for the poor reviewer
edit: (not just here, the whole PR needs reformatting)
There was a problem hiding this comment.
It would be nice not to give this an x86-specific name and meaning. Also, this setup won't play nice with package precompilation on a cluser – we need a slightly more advanced version that compiles julia once for all architectures to handle that case. We may need to consider disabling cache-compile in that situation.
| @@ -95,11 +95,17 @@ julia-ui-release julia-ui-debug : julia-ui-% : julia-src-% | |||
| julia-inference : julia-base julia-ui-$(JULIA_BUILD_MODE) $(build_prefix)/.examples | |||
| @$(MAKE) $(QUIET_MAKE) -C $(BUILDROOT) $(build_private_libdir)/inference.ji JULIA_BUILD_MODE=$(JULIA_BUILD_MODE) | |||
|
|
|||
| ifneq ($(CPUID_SPECIFIC_BINARIES), 0) | |||
| CPUID_TAG = _$(shell $(JULIA_EXECUTABLE) --cpuid) | |||
There was a problem hiding this comment.
needs $(call spawn
and should have a comment that this is being lazy evaluated (rather than using :=)
Why not? This PR adds a separate directory for each architecture. It's no worse than the current setup. |
|
It also doesn't fix the issues with the current setup, but introduces new ones. #14995 (comment) |
|
You're going to have to be more specific. This commit, even in its current form, is enormously helpful on a networked cluster. I'd be happy to see a better version, but I don't see the problem with putting this in as an option until such time. |
| @@ -83,6 +83,11 @@ HAVE_SSP := 0 | |||
| WITH_GC_VERIFY := 0 | |||
| WITH_GC_DEBUG_ENV := 0 | |||
|
|
|||
| # When set, give julia binaries CPUID specific names. This is usefull in cluster environments | |||
| # with heterogeneous architectures. N.B.: Binaries, will not be automatically rebuilt for all | |||
There was a problem hiding this comment.
useful has one "l" and the comma after "Binaries" is unnecessary
| @@ -241,6 +243,11 @@ int wmain(int argc, wchar_t *argv[], wchar_t *envp[]) | |||
| argv[i] = (wchar_t*)arg; | |||
| } | |||
| #endif | |||
| if (argc >= 2 && strcmp((char *)argv[1], "--cpuid") == 0) { | |||
| /* Used by the build system to name CPUID-specific binaries */ | |||
| printf("%llx", jl_cpuid_tag()); | |||
There was a problem hiding this comment.
C:/projects/julia/ui/repl.c:180:17: warning: 'jl_cpuid_tag' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
extern uint64_t jl_cpuid_tag();
^
C:/projects/julia/ui/repl.c: In function 'wmain':
C:/projects/julia/ui/repl.c:248:9: warning: unknown conversion type character 'l' in format [-Wformat=]
printf("%llx", jl_cpuid_tag());
^
C:/projects/julia/ui/repl.c:248:9: warning: too many arguments for format [-Wformat-extra-args]
|
Why did this get merged? I thought I was pretty clear this seemed like a bad idea. |
|
|
|
All of the review comments from before the merge were addressed. This works well for the use case it's there for. It's an optional feature that can be removed as soon as you provide something better. You didn't give a good reason with sufficient detail why it shouldn't be merged. |
Implements #19481. cc @andreasnoack for testing