COMP: Remove hardcoded -march=corei7, -mtune=generic, and dead code#6049
Conversation
|
| Filename | Overview |
|---|---|
| CMake/ITKSetStandardCompilerFlags.cmake | Removes -march=corei7 and -mtune=generic from non-MSVC x86/x86_64 GCC/Clang paths, deletes now-dead ICC detection code, and documents the rationale inline. Logic is correct; c_flags/cxx_flags placeholders remain but are harmless. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[check_compiler_optimization_flags] --> B{x86_64 / AMD64?}
B -- No --> Z[c_and_cxx_flags = empty]
B -- Yes --> C{MSVC?}
C -- Yes --> D[check_avx_flags\n+ /arch:SSE /arch:SSE2 if 32-bit]
C -- No --> E{NOT EMSCRIPTEN\nOR WASI?}
E -- Yes --> F["InstructionSetOptimizationFlags = empty\n(was: -march=corei7 -mtune=generic)\n(was also: dead ICC detection)"]
E -- No --> Z
D --> G[c_and_cxx_flags = InstructionSetOptimizationFlags]
F --> G
G --> H[check_c_compiler_flags\ncheck_cxx_compiler_flags]
H --> I[Return c / cxx optimization flags to caller]
Reviews (1): Last reviewed commit: "COMP: Remove hardcoded -march=corei7 and..." | Re-trigger Greptile
|
I agree with this packager need specific flags ( and it can be tedious to disable hard coded one), and local developers using the native architecture is a good option for local executables. |
hjmjohnson
left a comment
There was a problem hiding this comment.
Close, but some changes need addressing.
a948fea to
bfbb4a9
Compare
Remove architecture-specific compiler flags and dead code from the non-MSVC x86_64 path in ITKSetStandardCompilerFlags.cmake: - -march=corei7: recent GCC removed corei7 as an option (InsightSoftwareConsortium#2634) - -mtune=generic: compiler default, always a no-op - Intel ICC detection (icc/icpc): EOL 2023, replaced by ICX/Clang - Empty c_flags/cxx_flags placeholder variables: never populated No architecture flags are set by default, ensuring maximum redistributability. Added AVX-512 frequency throttling caution for users who add -march=native. Closes InsightSoftwareConsortium#2634.
bfbb4a9 to
7c0bfe8
Compare
The check_avx_flags() function had a CMake scoping bug: it set a local
variable `avx_flags_var` instead of using `set(${avx_flags_var} ...
PARENT_SCOPE)`, so the caller's InstructionSetOptimizationFlags was
never modified. The MSVC AVX/AVX2 runtime detection ran but its
results were silently discarded.
Remove:
- check_avx_flags() function definition (72 lines)
- Its sole call site in the MSVC branch
- The MSVC 32-bit /arch:SSE /arch:SSE2 append (also dead — appended
to the unmodified InstructionSetOptimizationFlags)
- The InstructionSetOptimizationFlags variable and c_and_cxx_flags
intermediary (both always empty on all platforms)
The check_c_compiler_flags/check_cxx_compiler_flags calls now receive
no architecture flags, matching the actual behavior that has been
in effect since the function was introduced.
Note: check_sse2_flags() is a separate function with a different
(also broken) scoping pattern but is still called elsewhere — left
for a follow-up.
Remove architecture-specific compiler flags that are unnecessary or broken, and dead Intel ICC detection code. Closes #2634. Supersedes #6039.
What was removed and why
-march=corei7corei7as an-marchoption (#2634). Targeted Nehalem (2008).-mtune=generic-mtuneis specified — always a no-op.icc/icpc)InstructionSetOptimizationFlagsto empty — same as the default.No architecture flags are set by default, ensuring maximum redistributability for pip wheels, Docker images, and hardware translation (Rosetta, QEMU). Users building for local performance should add
-march=nativeviaCMAKE_C_FLAGSorCMakeUserPresets.json.Added a CMake comment cautioning that
-march=nativeon AVX-512 CPUs should be paired with-mprefer-vector-width=256to avoid Intel frequency throttling.