Conversation
a99f5f8 to
f3383cf
Compare
|
I think we should split out the change to make |
Record once that the file defines variables for the scripts that source it, so their uses are invisible to static analysis, and quote the two expansions flagged for word splitting. Neither has an effect. Quoting the array element in shquot also stops an argument holding a tab or a newline from being split into several quoted words. That output is meant to be pasted back into a shell, where the split silently alters the command. AI-assisted.
Sounds reasonable, see: #2578 edit: TBH I'm interested to see how clever GH is and have rebased onto that new PRs commit. |
Merge a delimited variable with a list of inputs, skipping empty ones and avoiding dangling delimiters. Prepends by default so new inputs take priority. AI-assisted.
Fix directory determination used during the aomp_common_vars sourcing. Derive ROCM_PATH from AOMP. Export PATH and LD_LIBRARY_PATH such that AOMP and ROCM_PATH take precedence. Refactor CMake configuration to allow easy printing and add corresponding print. Log the CMake configure step. AI-assisted.
Search a list of prefixes for lib/cmake/<Module> and return the cmake directory holding it. Defaults to AOMP, then ROCM_PATH, then /opt/rocm, so a package missing from the ROCm under test resolves to the system ROCm. AI-assisted.
ggml needs hip, hipblas and rocblas. Look each of them up separately so the ROCm under test is preferred and another installation is only added to CMAKE_PREFIX_PATH for what it does not ship itself, with a warning naming it. AI-assisted.
A ROCm package borrowed from another installation exports its whole include directory, HIP headers included, which CMake emits as -isystem. Since clang searches its own ROCm last, those headers win over the ROCm under test. Emit the include directory of the ROCm under test through CMAKE_<LANG>_COMPILER_ARG1, the only slot ahead of the includes CMake generates; every *_FLAGS variable lands behind them. AI-assisted.
Without this the HIP headers of whichever ROCm provides hipblas and rocblas are used, which fails to compile whenever they predate the compiler under test. AI-assisted.
Confirm the include directory survived into the CMake cache. A lost entry only breaks the build while the two ROCm header sets are incompatible; otherwise it silently builds against the wrong ones. AI-assisted.
test-backend-ops alone runs for about 2500s on MI350X and is killed by the 1500s ctest applies by default, which reports as a test failure rather than as an unfinished test. AI-assisted.
AI-assisted.
f3383cf to
4b6743a
Compare
| # CMAKE_<LANG>_COMPILER_ARG1 is the only slot in front of those includes; every | ||
| # *_FLAGS variable lands behind them. Keep CMAKE_<LANG>_COMPILER a plain path, | ||
| # CMake reassigns ARG1 when the compiler itself carries arguments. | ||
| function add_cmake_rocm_header_priority_args() { |
There was a problem hiding this comment.
When using a compiler-only "AOMP", e.g. hipblas will be discovered within the system's ROCm installation which adds an particular include path: -isystem /path/to/rocm/include.
Primarily some incompatible ggml header files provided by the llama repo itself are picked up.
This has the effect that these potentially incompatible headers take precedence over our desired header files / the header files provided by our tested compiler toolchain.
Hence, this _ARG1 is the "hammer" which brings our include directory in front and lets our compiler's header files take precedence.
This avoids e.g. error: use of undeclared identifier '__ocml_log2_f32'.
If you happen to know a better approach, please let me know.
Also I am slightly unsure what's going on exactly.
AFAICT -isystem seems intended as third-party header include, so one would lean towards using -I.
But switching this added include from -isystem to -I fails with the same errors.
2 participants
Motivation
Observed fails in the CI, missing CTest instances and missing performance results.
Technical Details
Expanded common function toolbox by:
These are being used to handle:
PATHandLD_LIBRARY_PATHexpansions, needed to run llamacpp-isystempulled in by system ROCmCMAKE_HIP_COMPILER_ARG1A could cause wrong libraries being discovered first. (warning)
B could cause missing CMake configuration file. (error)
C could cause system header taking precedence over targeted / tested compiler toolchain. (error)
Also expanded CTest timeout to accommodate
test-backend-opsinstance.Test Plan
Run llamacpp via CI scripts.
Test Result
Full, expected suite of CTests shows up in addition to performance results.
Submission Checklist