Skip to content

feat(rocSHMEM): accelerate build scripts, #AIROCSHMEM-563 - #15070

Merged
amirsojoodi merged 2 commits into
developfrom
users/asojoodi/rocshmem-accelerate-build
Nov 6, 2026
Merged

amirsojoodi merged 2 commits into
developfrom
users/asojoodi/rocshmem-accelerate-build

Conversation

@amirsojoodi

Copy link
Copy Markdown
Contributor

Motivation

  • scripts/build_configs/{all_backends,gda,ipc_single,ipc_tests_only,ro_ipc,ro_net}: replaced hardcoded cmake --build . --parallel 8 with cmake --build . --parallel ${BUILD_JOBS:-$build_jobs}, where build_jobs defaults to min($(nproc), 32) — uses more of the machine without defaulting to monopolizing a large shared dev box, still fully overridable via BUILD_JOBS.
  • Same six scripts: now pass -G Ninja to the cmake configure invocation only when ninja is found on PATH, falling back to CMake's own default generator otherwise. Investigation of .ninja_log showed the per-GPU-arch bitcode opt/llvm-link steps (cmake/DeviceBitcode.cmake) are structurally independent across architectures but weren't reliably scheduled in parallel under Unix Makefiles; Ninja schedules them correctly.
  • Added scripts/build_configs/_common.sh, sourced by all six scripts, holding the shared ASAN/HSA_XNACK, generator-detection, and job-count logic that was previously duplicated verbatim across every script.

Issue Tracking

JIRA ID: AIROCSHMEM-563

@amirsojoodi
amirsojoodi requested a review from a team as a code owner November 4, 2026 22:42
Copilot AI balanced review requested due to automatic review settings November 4, 2026 22:42
@therock-pr-bot

therock-pr-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Auto-selecting Ninja in an existing Unix Makefiles build directory causes a CMake generator mismatch and prevents the scripts from building.

1 open issue
What changed in this PR

This PR aims to speed up rocSHMEM builds by improving parallelism and sharing setup across six build configurations.

Changes:

  • Prefer Ninja when available, otherwise use CMake’s default generator.
  • Replace the fixed eight build jobs with the CPU count capped at 32, overridable through BUILD_JOBS.
  • Centralize ASAN setup, generator detection, and job-count logic in _common.sh.
File Description
projects/​rocshmem/​scripts/​build_configs/​ro_net Apply shared setup and configurable parallelism to network builds.
projects/​rocshmem/​scripts/​build_configs/​ro_ipc Apply shared setup and configurable parallelism to RO IPC builds.
projects/​rocshmem/​scripts/​build_configs/​ipc_tests_only Apply shared setup and configurable parallelism to IPC test builds.
projects/​rocshmem/​scripts/​build_configs/​ipc_single Apply shared setup and configurable parallelism to single-node IPC builds.
projects/​rocshmem/​scripts/​build_configs/​gda Apply shared setup and configurable parallelism to GDA builds.
projects/​rocshmem/​scripts/​build_configs/​all_backends Apply shared setup and configurable parallelism to all-backend builds.
projects/​rocshmem/​scripts/​build_configs/​_common.sh Add shared ASAN, generator-selection, and job-count logic.

Review effort: Balanced

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (4gfx1201)

Dashboard artifact

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (4gfx1201)

Dashboard artifact

@peymanr

peymanr commented Nov 5, 2026

Copy link
Copy Markdown

PR Pundit review

(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)

# Finding Where
1 🟠 Exported CMAKE_GENERATOR silently ignored on fresh builds: this PR injects -G Ninja projects/rocshmem/scripts/build_configs/_common.sh:33
2 🟠 Quota-limited rocSHMEM builds OOM-killed: this PR replaced --parallel 8 with min(nproc,32) projects/rocshmem/scripts/build_configs/_common.sh:37
3 🟠 Nested RCCL sub-build oversubscribes with 32 rocSHMEM jobs: this PR raised the default and RCCL sets no BUILD_JOBS projects/rccl/cmake/ROCSHMEM.cmake:166

1. 🟠 Should fix: Exported CMAKE_GENERATOR silently ignored on fresh builds: this PR injects -G Ninja

projects/rocshmem/scripts/build_configs/_common.sh:33

_common.sh adds -G Ninja whenever ninja is on PATH and the cwd has no CMakeCache.txt. Because a command-line -G beats the environment variable, an exported CMAKE_GENERATOR (e.g. Unix Makefiles, Ninja Multi-Config) is silently ignored on a fresh build dir. The scripts build and install themselves through cmake --build/cmake --install, and the docs never tell users to run make there, so the build still works. A user can still pick a generator by passing -G ... as a script argument, because the later -G wins. Consider skipping the injection when CMAKE_GENERATOR is set.

Who is affected: Users or CI that export CMAKE_GENERATOR and run a rocSHMEM build_configs script in a fresh build directory on a host with ninja installed.

Checked against the code by an independent pass.

2. 🟠 Should fix: Quota-limited rocSHMEM builds OOM-killed: this PR replaced --parallel 8 with min(nproc,32)

projects/rocshmem/scripts/build_configs/_common.sh:37

The default build parallelism changes from a fixed --parallel 8 to min(nproc, 32). nproc follows CPU affinity but ignores cgroup CPU quotas and memory limits, so a quota-limited container still starts up to 32 memory-heavy hipcc device compiles. That can thrash or OOM where 8 jobs used to work. Note: if nproc fails, set -e aborts the script, so there is no unbounded --parallel. Ignoring CMAKE_BUILD_PARALLEL_LEVEL is not new, since the base also passed an explicit --parallel 8. Consider honouring the cgroup quota or keeping a conservative default when BUILD_JOBS is unset.

Who is affected: Anyone running the rocSHMEM build_configs scripts without BUILD_JOBS in a CPU-quota-limited container on a many-core host: the Dockerfile, RCCL CI build-rocshmem.sh, and RCCL's ROCSHMEM.cmake ExternalProject, which nests inside the outer ninja build.

Checked against the code by an independent pass.

Related past reviews: #7175 on .github/workflows/rocjitsu-corpus-tests.yml, #199 on projects/rccl/CMakeLists.txt

3. 🟠 Should fix: Nested RCCL sub-build oversubscribes with 32 rocSHMEM jobs: this PR raised the default and RCCL sets no BUILD_JOBS

projects/rccl/cmake/ROCSHMEM.cmake:166

The BUILD_COMMAND in RCCL's ROCSHMEM.cmake:166 runs build_configs/gda while the outer RCCL build is still compiling. The inner build now uses min(nproc, 32) jobs instead of the old fixed 8. On hosts with more than 8 CPUs this raises nested oversubscription (outer -j plus up to 32 rocSHMEM device compiles), adding CPU and RAM pressure. The old --parallel 8 did not share a jobserver either, so the change is in degree only. Consider passing BUILD_JOBS= from the RCCL side, or a parent-derived job count, on this nested path.

Who is affected: Developers or CI building RCCL with --rocshmem/--rocshmem-gin (rocSHMEM from source, no ROCSHMEM_INSTALL_DIR) on machines with more than 8 CPUs.

Checked against the code by an independent pass.

Related past reviews: #199 on projects/rccl/CMakeLists.txt

Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at 7339b239; findings checked and posted by @peymanr; points already raised on this PR are left out.

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@amirsojoodi
amirsojoodi merged commit 508cc8f into develop Nov 6, 2026
60 of 62 checks passed
@amirsojoodi
amirsojoodi deleted the users/asojoodi/rocshmem-accelerate-build branch November 6, 2026 15:25
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants