Skip to content

[compiler] build tools for code cov - #4931

Merged
reboss merged 6 commits into
ROCm:mainfrom
reboss:code-cov-tools
Jun 10, 2026
Merged

reboss merged 6 commits into
ROCm:mainfrom
reboss:code-cov-tools

Conversation

@reboss

@reboss reboss commented Apr 29, 2026

Copy link
Copy Markdown
Member

Motivation

For code coverage jobs, we'll need to have these tools available. It would be nice if they were already pre-built, so my goal for this PR is to make TheRock build aware of these tools and then publish them to be consumed in CI, without having to build them from scratch every time.

Technical Details

Just updates the CMake flow to build these artifacts.

Test Plan

Test Result

Submission Checklist

reboss and others added 3 commits April 29, 2026 10:35
Add explicit flags to build llvm-cov and llvm-profdata tools:
- LLVM_TOOL_LLVM_COV_BUILD=ON
- LLVM_TOOL_LLVM_PROFDATA_BUILD=ON

These tools are required for code coverage builds in rocm-libraries
projects (hipDNN, rocPRIM, etc.) that use -fprofile-instr-generate
and -fcoverage-mapping flags.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Include LLVM_COV and LLVM_PROFDATA in the _llvm_required_tools list
so these tools are built and installed even when THEROCK_ENABLE_LLVM_TESTS
is OFF.

Without this, the pre_hook explicitly disables all tools not in the
required list, preventing llvm-cov and llvm-profdata from being built
despite the CMake flags in CMakeLists.txt.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Include LLVM_CXXFILT in the _llvm_required_tools list as it's required
by hipDNN's coverage build configuration.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Comment on lines +184 to +192
LLVM_COV
LLVM_CXXFILT
LLVM_DIS
LLVM_DWARFDUMP
LLVM_LINK
LLVM_MC
LLVM_NM
LLVM_OFFLOAD_BINARY
LLVM_PROFDATA

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be working, see

@reboss
reboss marked this pull request as ready for review May 4, 2026 17:32
@marbre
marbre self-requested a review May 4, 2026 17:53

@geomin12 geomin12 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.

looks like it is working but i'll let other folks review this for build

@jayhawk-commits

Copy link
Copy Markdown
Contributor

The build failures seem infra-related, so retrying those failed jobs.

@jayhawk-commits jayhawk-commits 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.

LGTM, with the produced artifacts including the added tools

Looping in compiler team for awareness. CC @ronlieb @skganesan008

@jayhawk-commits

Copy link
Copy Markdown
Contributor

The S3 upload issues from this fork for the native packaging potentially fixed when #5066 lands. More concerned about the signal from the Windows build at this time.

@reboss
reboss merged commit e51f242 into ROCm:main Jun 10, 2026
40 of 43 checks passed
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Jun 10, 2026
@ashuverm

Copy link
Copy Markdown

@reboss , No JIRA information available in this PR, and the Test Plan / Test Result sections were left empty. Please add for traceability.

No automated tests were added with this PR. Verification that llvm-cov, llvm-profdata, and llvm-cxxfilt are produced was done manually (artifact inspection in review). The existing structural test — tests/test_artifact_structure.py — only checks invariants (no cross-artifact overlaps, manifest present); it does not assert that these specific binaries ship. Since packaging is glob-based (lib/llvm/bin/**), a future change that drops a tool from _llvm_required_tools would silently stop publishing it with no CI failure.

Since this is already merged, as a follow-up can you add a unit test (or extend test_artifact_structure.py) covering the below scenarios:

  • The amd-llvm run artifact contains lib/llvm/bin/llvm-cov
  • The amd-llvm run artifact contains lib/llvm/bin/llvm-profdata
  • The amd-llvm run artifact contains lib/llvm/bin/llvm-cxxfilt

This guards the intent of this PR — that the coverage tools remain present in the published artifact for downstream code-coverage jobs (e.g. ROCm/rocm-libraries#4441).

@ScottTodd

Copy link
Copy Markdown
Member

No JIRA information available in this PR

Work tracking for open source code belongs on GitHub. Mirroring to Jira is optional, and TheRock uses GitHub exclusively.

No automated tests were added with this PR.

I'm all for adding test coverage, but we don't currently test that every LLVM tool is built, only those that are user-facing.

See the list of what is built here:

set(_llvm_required_tools
LLVM_AR
LLVM_AS
LLVM_CONFIG
LLVM_COV
LLVM_CXXFILT
LLVM_DIS
LLVM_DWARFDUMP
LLVM_LINK
LLVM_MC
LLVM_NM
LLVM_OFFLOAD_BINARY
LLVM_PROFDATA
LLVM_SHLIB
LLVM_OBJCOPY
LLVM_OBJDUMP
LLVM_READOBJ
LLVM_SYMBOLIZER
OPT
YAML2OBJ
)

And what we have tests for here:

# Console script tests are templated across tuples of
# (script_name, cl, expected_text, required)
# For example:
# ("hipcc", ["--help"], "clang LLVM compiler", True)
# This will run `hipcc --help` and check the output for "clang LLVM compiler".
# If the script does not exist, the test case will fail.
COMMON_CONSOLE_SCRIPT_TESTS = [
("amdclang", ["--help"], "clang LLVM compiler", True),
("amdclang++", ["--help"], "clang LLVM compiler", True),
("amdclang-cpp", ["--help"], "clang LLVM compiler", True),
("amdclang-cl", ["-help"], "clang LLVM compiler", True),
("amdflang", ["--help"], "LLVM compiler", True),
("amdlld", ["-flavor", "ld.lld", "--help"], "USAGE:", True),
("hipcc", ["--help"], "clang LLVM compiler", True),
("hipconfig", [], "HIP version:", True),
("hipify-clang", ["--help"], "USAGE:", True),
]

The "tests" for these internal tools getting built is that they are used within the build/test systems.

The artifact structure test would be the wrong place to add coverage anyways. I linked to the user-facing install tests above, and project-specific tests would go in either

if(THEROCK_BUILD_COMGR_TESTS)
if(WIN32)
set(_llvm_lit_script "${CMAKE_BINARY_DIR}/compiler/amd-llvm/build/bin/llvm-lit.py")
else()
set(_llvm_lit_script "${CMAKE_BINARY_DIR}/compiler/amd-llvm/build/bin/llvm-lit")
endif()
therock_cmake_subproject_build_test(amd-comgr
COMMAND
"${Python3_EXECUTABLE}" "${_llvm_lit_script}"
"${CMAKE_BINARY_DIR}/compiler/amd-comgr/build/test-lit" -v
COMMAND
"${CMAKE_CTEST_COMMAND}"
--test-dir "${CMAKE_BINARY_DIR}/compiler/amd-comgr/build"
--output-on-failure
)
endif()
(lit tests for the compiler itself) or https://github.com/ROCm/TheRock/tree/main/build_tools/github_actions/test_executable_scripts (which has no LLVM tests currently)

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants