zjw1111 commented on code in PR #181:
URL: https://github.com/apache/paimon-cpp/pull/181#discussion_r3719335059
##########
cmake_modules/SetupCxxFlags.cmake:
##########
@@ -22,6 +22,30 @@ include(CheckCXXCompilerFlag)
message(STATUS "System processor: ${CMAKE_SYSTEM_PROCESSOR}")
+include("${CMAKE_CURRENT_LIST_DIR}/TargetArchitecture.cmake")
+message(STATUS "Target processor: ${PAIMON_TARGET_PROCESSOR} "
+ "(CPU family: ${PAIMON_TARGET_CPU_FAMILY})")
+
+# Probe the architecture specific tuning flags. x86 deliberately gets none of
its
+# own, so there is nothing to probe there: the baseline stays whatever the
+# toolchain defaults to, and no persisted value depends on it; see
+# src/paimon/common/utils/crc32c.h.
+if(PAIMON_TARGET_CPU_FAMILY STREQUAL "ppc")
Review Comment:
Thanks for the thorough cleanup of the architecture detection here.
I wonder if the ppc handling is still worth keeping. paimon-cpp does not
support building for ppc: the `docs/code-style.md` update in this very PR
states that only x86_64 and aarch64 are supported, ppc is not built or tested
in CI, and `build_lumina` raises a `FATAL_ERROR` for any platform outside
`linux-x86_64`.
On top of that, the ppc branch cannot take effect even on a ppc host.
`PAIMON_ALTIVEC` is never defined anywhere in the repository — there is no
`define_option` for it — so the `if(CXX_SUPPORTS_ALTIVEC AND PAIMON_ALTIVEC)`
guard further down is always false, and this new `-maltivec` probe only feeds a
variable that is never read. That is the same reason the x86 `PAIMON_CPU_FLAG`
/ `PAIMON_SIMD_LEVEL` block was removed in this PR, so it seems a little
inconsistent to keep ppc.
Would it be possible to drop the ppc pieces altogether in this PR — this
probe block, the `PAIMON_TARGET_CPU_FAMILY STREQUAL "ppc"` block further down
in this file, and the ppc mapping in `cmake_modules/TargetArchitecture.cmake`?
Unknown processors already fall through to `unknown` without an error, so ppc
hosts would keep configuring with toolchain defaults, exactly as they
effectively do today. If you would rather keep the scaffolding for a future
port, that is fine too — in that case adding the missing `PAIMON_ALTIVEC`
option would at least make the branch reachable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
