Re: [PATCH v2] perftest: add a ROCm build to CI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perftest: unused matrix.rocm_arch in CI workflow and commit message mismatch
--

commit f80ce07d3d5d2a3f4581f598c3afcf67e5214052
Author: Stephen Bates <sbates@raithlin.com>

perftest: add a ROCm build to CI

This commit adds a GitHub Actions workflow on Ubuntu 24.04 to build and
install perftest with ROCm and ROCm DMA-BUF support enabled inside AMD's
slim ROCm container image.

Link: https://github.com/linux-rdma/perftest/pull/418

>     perftest: add a ROCm build to CI
>     
>     CI builds only the CUDA configuration. The two existing workflows pass no
>     --enable-rocm, so src/rocm_memory.c and everything behind HAVE_ROCM and
>     HAVE_ROCM_DMABUF is never compiled on a pull request, and a change that
>     breaks the ROCm backend goes green.
>     
>     Add an Ubuntu 24.04 job building --enable-rocm --enable-rocm-dmabuf,
>     alongside the two CUDA jobs and triggered the same way, as a matrix over
>     gfx950 (Instinct MI355X, CDNA4) and gfx1250 (Instinct MI455X, CDNA5). Those
>     two were picked to cover consecutive Instinct generations: HIP sources that
>     compile correctly for one generation may have microarchitecture-specific issues
>     on the next, and testing both catches that. fail-fast is off: a failure is far
>     more likely to be specific to one generation than common to both, and cancelling
>     the other leg hides exactly the comparison that would say so.
>     
>     It runs in AMD's slim ROCm image rather than installing ROCm on the runner.
>     The -full and -complete tags, and an apt install of the amdrocm-core
>     metapackages, all pull the whole ML stack: Composable Kernel alone is over
>     2 GB, before rocSHMEM, MIOpen and the BLAS and FFT libraries. A compile check
>     touches none of it. rocm/dev-ubuntu-24.04:7.2.4 is about 1.1 GB against 8.3 GB
>     for the smallest 10.1 image, and still ships hipcc, the HIP and HSA headers,
>     and device support for both matrix targets -- llc -mcpu=help lists gfx950 and
>     gfx1250, and both compile.
>     
>     7.2.4 rather than 10.1 because AMD publishes slim tags only through 7.2.x;
>     every 10.x tag is -full. What this job needs is a toolchain that compiles the
>     sources for the targets in the matrix, which that image provides. Moving to a
>     newer ROCm is a one-line change to the container tag if a slim 10.x image
>     appears, or if something here starts depending on a newer HIP API.
>     
>     Like the CUDA jobs this builds and installs only. There is no RDMA device in
>     a runner, and the project has no test suite.
>     
>     Verified by running this workflow in a fork of the repository across both
>     matrix architectures. Both legs passed: configure accepts every option, and
>     make and make install succeed warning-free.
>     
>     Link: https://github.com/linux-rdma/perftest/pull/418
>     Signed-off-by: Stephen Bates <sbates@raithlin.com>
>
> diff --git a/.github/workflows/ubuntu24_04_rocm7_2.yaml b/.github/workflows/ubuntu24_04_rocm7_2.yaml
> new file mode 100644
> index 0000000000000..70790377d85a0
> --- /dev/null
> +++ b/.github/workflows/ubuntu24_04_rocm7_2.yaml
> @@ -0,0 +1,70 @@

[ ... ]

> +    strategy:
> +      fail-fast: false
> +      matrix:
> +        # rocm_arch is not yet passed to configure: --with-rocm-arch only
> +        # exists once HIP kernel sources are added. Both legs build identically
> +        # until then; the matrix is in place so the workflow does not need
> +        # amending when device code arrives.
> +        rocm_arch: [gfx950, gfx1250]
> +
> +    name: ROCm (${{ matrix.rocm_arch }})

[ ... ]

> +      - name: Configure the build
> +        run: |
> +          ./configure --enable-rocm --enable-rocm-dmabuf \
> +                      --with-rocm="$ROCM_PATH"

[Severity: Medium]
As reported by Sashiko on v1 [1], in .github/workflows/ubuntu24_04_rocm7_2.yaml,
jobs.build.strategy.matrix defines rocm_arch: [gfx950, gfx1250], but
matrix.rocm_arch is only interpolated into the job display name and is never
passed to the environment, ./configure, or make in jobs.build.steps.

The new comment in jobs.build.strategy.matrix notes:

        # rocm_arch is not yet passed to configure: --with-rocm-arch only
        # exists once HIP kernel sources are added. Both legs build identically
        # until then; the matrix is in place so the workflow does not need
        # amending when device code arrives.

However, because the "Configure the build" step in jobs.build.steps invokes:

          ./configure --enable-rocm --enable-rocm-dmabuf \
                      --with-rocm="$ROCM_PATH"

without passing matrix.rocm_arch, won't this workflow file still need to be
amended anyway when --with-rocm-arch and device code arrive? In the meantime,
every push and pull request against master pulls the container image twice,
installs all packages twice, and runs two identical host-only builds.

In addition, does the commit message still match what this workflow actually
does? The commit message states that the job builds a matrix over gfx950 and
gfx1250 to catch microarchitecture-specific HIP compilation issues across
consecutive Instinct generations, that fail-fast is off because a failure is
likely specific to one generation, and that both targets compile in this job,
whereas the workflow currently compiles neither target architecture.

Could the workflow run a single build job until HIP kernel sources and
--with-rocm-arch support are introduced, and could the commit message be
updated to match?

[1] https://lore.kernel.org/r/20261104155023.250706-1-sbates@raithlin.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104203301.1677350-1-sbates@raithlin.com?part=1