Fix malfunctioning AVX512 check in getarch #3571
Conversation
There was a problem hiding this comment.
I'm not convinced that this will work: TARGET_MAKE will be created by Makefile.prebuild - it must be available at the time Makefile.prebuild is read, not when the rule executes (which would not be the case anyway). Therefore, it will not be available when Makefile.prebuild is run for the first time (with the same setting of TARGET_CORE).
If nothing else, this adds to the confusion around Makefile.prebuild. It would be nice if the goal was to make the build process more transparent.
|
|
There was a problem hiding this comment.
Why not simply reuse c_check and call it with:
ifeq (1, $(shell /usr/bin/env perl c_check - - $(CC) $(TARGET_FLAGS) $(CFLAGS) | awk '/NO_AVX512.*/{split($0,a,"="); print a[2];}'))
in Makefile.prebuild.
Adding more scripts that may potentially get out of sync with one another seems to be a recipe for errors down the road.
Or even simpler:
getarch : getarch.c cpuid.S dummy $(CPUIDEMU)
avx512=$$(perl c_check - - gcc | grep NO_AVX512); \
$(HOSTCC) $(HOST_CFLAGS) $(EXFLAGS) $${avx512:+-D$${avx512}} -o $(@F) getarch.c cpuid.S $(CPUIDEMU)
|
Right, thanks, indeed I would want this to be contained within Makefile.prebuild and no extra files. Just not sure if awk and split can be expected to be available everywhere, but a simple grep would probably do and is already used in Makefile.system. |
|
Right - in fact, after I wrote the first version of the comment it occurred to me that I could also have used perl (which we need already) - I just felt too lazy to hack up the command line. |
|
Hehe, that's cursed. |
|
While we are looking at Makefile.prebuild anyway: would be useful to let make know about the 'phony-ness' of this target. Here, the real build target is $(TARGET_CONF) not config.h. While the rules seem to work - thanks to the 'dummy' target - I wonder if there is a reason not to specify the build target explicitly: Unless there is a reason for not doing this I do not currently see. |
|
Superseding with #3579 now. |
@martin-frbg Indeed, DYNAMIC_ARCH is what I'm most interested in. Thus, my other patch creating separate Makefile_.conf and config_kernel_.h for improving debugging. I've got a patch for separating TARGET_CONF and adding a .phony target, Will look into this again. One further note: the test for avx512 support is a bit of a 'one off', that will not scale well: Cooperlake has introduced support for avx512bf16, which is only supported since gcc10. The older SkylakeX and the newer Sapphirerapids may also have introduced restrictions beyond avx512. Thus, a much more fine-grained separation would have to be in place with the appropriate fallbacks (like Cooperlake->SkylakeX->Haswell). I'm afraid this will not scale well with the present structure. |
|
Correct, the AVX512 test is neither expected nor intended to scale beyond ensuring basic AVX512 capability of the toolchain. It dates back to when the first Skylake Xeon models became available and some distributions still shipped with compilers as old as gcc4. The separation and fallback code is in cpuid_x86.c (for single target) and driver/others/dynamic.c (dynamic_arch case) where relevant CPUID is queried. |
I've not tried to pull-request a simple patch addressing both issues - unfortunately, the test suite fails for a build using cmake on OSX with the error message:
The patched file (Makefile.prebuild) should not be used at all here. I wonder why these builds are so fragile. |
|
Yes that's totally unrelated - a workaround for an OSX/CMAKE/gfortran linking issue with response files that is only working half of the time. Restarting the build will see it pass so I consider it an improvement over the original case of DYNAMIC_ARCH builds not working on OSX with CMAKE at all. |
|
@martin-frbg makes sense. After re-triggering, the build has succeeded, now. Thanks! |
As explained in #3557, the additional test introduced in #1980 to ensure that the compiler is capable of handling AVX512 instructions is broken. Rather than checking a compiler property, as the largely redundant test in c_check does, it added an unwanted dependency on the compile host. As it turns out, the CMAKE build was already making the state of the NO_AVX512 variable from c_check available to the getarch build, this PR adds the equivalent for gmake by parsing the initial Makefile.conf output from c_check.