gdb.rocm: replace hipcc with amdclang++ as the HIP compiler#62
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes the ROCm/HIP testsuite migration from hipcc to amdclang++ by updating compiler discovery, build flag handling, naming, and test/doc references to align with the current ROCm toolchain.
Changes:
- Switch HIP compiler discovery to
amdclang++(withHIP_COMPILER_FOR_TARGEToverride andHIPCC_FOR_TARGETfallback) and keep backward-compatible aliases. - Update HIP compilation/linking flags to correctly apply
-x hiponly when compiling sources and--hip-linkonly when producing executables. - Rename testsuite gating helpers (
allow_hipcc_tests→allow_hip_tests) and update testcases and documentation accordingly (with compatibility aliases retained).
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gdb/testsuite/lib/rocm.exp | Renames HIP gating proc to allow_hip_tests and keeps allow_hipcc_tests as a compatibility alias. |
| gdb/testsuite/lib/gdb.exp | Updates HIP compile/link flag handling for amdclang++ (-x hip / --hip-link) and related comments. |
| gdb/testsuite/lib/future.exp | Switches HIP compiler discovery to amdclang++, adds HIP_COMPILER_FOR_TARGET, and introduces new/compat proc names. |
| gdb/testsuite/gdb.rocm/watchpoint-basic.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/watchpoint-at-end-of-shader.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/watch-gpu-global-from-host.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/update-thread-list.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/until-tests.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/unaligned-memory-access.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/step-schedlock-spurious-waves.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/step-over-kernel-exit.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/static-global.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/simple.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/simple-outside-debugger.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/show-info.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/shared-memory.exp | Updates require gate to allow_hip_tests (and keeps python requirement). |
| gdb/testsuite/gdb.rocm/scheduler-locking.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/runtime-core.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/resume-exception.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/register-watchpoint.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/program-execution.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory-warning-watchpoint.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory-warning-sigsegv.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory-multi-inferiors.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory-fork.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/precise-memory-exec.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/ocp_mx.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/nonstop-mode.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/nonstop-displaced.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/names.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/multi-inferior-run-spurious-waves.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/multi-inferior-fork.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/mi-lanes.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/mi-attach.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/mi-aspace.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/load-core-remote-system.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/line-breakpoint-in-kernel.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/lane-pc-vega20.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/lane-info.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/lane-execution.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/instruction-stepping-commands.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/info-sharedlibrary.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/info-dispatches.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/illegal-insn-sigill.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/hip-builtin-variables.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/hip-builtin-completions.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/generic-address.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/gcore-after-attach.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/fork-exec-non-gpu-to-gpu.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/fork-exec-gpu-to-non-gpu.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/finish.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/exec-bit-extract.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/displaced-stepping.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/disassemble.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/devicecode-breakpoint.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/device-interrupt.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/device-barrier.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/device-attach.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/detach-while-breakpoints-inserted.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/deref-scoped-pointer.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/deep-stack.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/debugtrap.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/corefile.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/core-no-read-special-files.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/convenience_variables.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/continue-over-kernel-exit.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/breakpoint-after-exit.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/break-kernel-no-debug-info.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/branch-fault.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/bit-extract.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/aspace-watchpoint.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/aspace-user-input.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/alu-exceptions.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.rocm/addr-bp-gpu-no-deb-info.exp | Updates require gate to allow_hip_tests. |
| gdb/testsuite/gdb.perf/rocm-break-cond-false.exp | Updates require gate to allow_hip_tests for the perf testcase. |
| gdb/testsuite/boards/hip.exp | Updates HIP board config to use find_hip_compiler, renames allow proc, and adjusts amdclang++ detection. |
| gdb/doc/gdb.texinfo | Updates documentation example command line to amdclang++ -x hip --hip-link .... |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
19d5312 to
3635e5f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lumachad
left a comment
There was a problem hiding this comment.
Thanks! Looks mostly good to me. Made some comments about small adjustments.
lancesix
left a comment
There was a problem hiding this comment.
I only skimmed thought the patch at this point. Note that some of what you are touching comes from upstream, and we might want to land the changes there as well. I'd probably advise for having this changed upstream first, then we can import it into amd-staging.
|
I agree with upstreaming. We should keep the PR open for reviews and validation. Once we're happy with it, we can take it upstream, wait for approval and pick this up again, then close the PR. |
|
Another point is making sure this works on the Windows HIP SDK too. The compiler tool name and paths may be different there IIRC. The boards/hip.exp changes need to be tested with |
f938e5f to
12dad2a
Compare
Please make sure you test this. I tried it now with: $ make check RUNTESTFLAGS="--target_board=hip" TESTS="gdb.base/break.exp" and that fails with: Without your patch, it compiles OK. (It then fails for other unrelated reasons.) |
Fixed. The --target_board=hip path broke because amdclang++ (unlike hipcc) doesn't automatically discover the HIP headers -- it needs --rocm-path to locate them. Added --rocm-path=$rocm_path to the board's compilation flags, along with the -x hip / --hip-link split (compile vs link). Tested with make check RUNTESTFLAGS="--target_board=hip" TESTS="gdb.base/break.exp" -- compilation succeeds, zero hip_runtime.h errors. |
@palves Loop over $source and recursively call gdb_compile to produce a .o for each source file (with -x hip, since they're sources) |
I'm confused. When running the normal gdb.rocm/ tests, I don't see "--rocm-path" passed to amdclang++. Why is it needed here, and not there? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Bringing this up here. @amd-shahab was asking questions on finding llvm-dwarfdump on a CI system. I think we should take the opportunity to make the tools search order correct in these changes (or a separate patch if desired). See my previous comment here: #62 (comment). I think we should honor PATH first. Then if ROCM_PATH is set, we go look for tools there. Then if everything fails, we search some hardcoded /opt/rocm path. |
ed95b99 to
dff4717
Compare
I agree we should look into this, though I'd prefer if any changes to the search path order was done in a separate patch. We have a little mess here, with search orders different for different tools. E.g., "${rocm_path}/llvm/bin/clang-offload-bundler" in rocm.exp (which now misses the leading lib/), Shahab's patch that searches PATH first, the searches for all the different tools lib/future.exp which search PATH last. |
Well, I think that change went too far. I had only asked to remove the comments parts. hipcc still exists and is widely used by users, so I think that part of the manual should have stayed. |
|
What is that "--genco" commit about? If you do fixup commits, please make sure to add some rationale in their commit logs. That was needed because of these two tests, I guess: testsuite/gdb.rocm/code-object-load-while-breakpoint-hit.exp: { debug hip additional_flags=--genco additional_flags=-DDEVICE } ] != "" } { But why the mapping instead of just fixing the tests? |
703913c to
69e9970
Compare
Agreed, that was overreach on my part. I've restored the
Good point on both counts. I've dropped the
I also folded everything back into the single commit (no more fixup commit), and the commit log now documents the |
There was a problem hiding this comment.
The patch itself LGTM now. Just a few comments on the commit log:
- The first paragraph has a few nits to pick:
The GDB testsuite used hipcc for HIP compiler discovery, naming, and
documentation, even though the underlying compiler is amdclang++.
Complete the switch to amdclang++ throughout the testsuite.
"The GDB testsuite used hipcc" => "uses hipcc"
The "the underlying compiler is amdclang++" isn't what motivates this. The real reason is that hipcc is deprecated and is going away at some point.
And then "complete the switch" implies the switch had been started already. It hasn't, this patch starts and finishes it.
-
It doesn't mention the -fdiagnostics-color change in the "Compile and link flags" section. It should mention that amdclang++ is clang-based and accepts the flag.
-
"Add HIP_COMPILER_FOR_TARGET" is not strictly correct, it's more rename "HIPCC_FOR_TARGET to HIP_COMPILER_FOR_TARGET".
-
FYI, the PR text and the commit log are slightly out of sync, which is a bit confusing and made me wonder which one is up to date. Since with "squash and merge" the PR text doesn't make it to git (the git log history is the canonical one), then feel free to simplify the PR text to give the short intro and then say to look at the individual commit log for details. Alternatively, it's fine to have the info in both places, though I'd suggest using the gh tool to easily and automatically sync them.
|
Sorry I have not had much time to look at this PR. I expect a fair amount of this can / should be upstream, right? Having this only downstream would cause some divergence we'll need to fix soon. |
Agreed, you had already mentioned this earlier, even. :-) One issue is that upstream does not have any of the Windows bits in the testsuite (which needed adjustment in this patch), and it has fewer tests to adjust. This is why I kept pushing for finishing up the patch downstream first. I'd vote for landing the patch downstream first, and then also send it upstream shortly after. The upstream version will end up slightly simplified in some parts. |
oups, forgot I already mentioned that 😅
cool, works for me. |
69e9970 to
5d59915
Compare
Thanks - addressed all of these in the latest force-push:
|
palves
left a comment
There was a problem hiding this comment.
Thanks, LGTM. Let's merge this (with rebase and merge).
Next step is upstreaming this. Let me/us know if you'd like to work on that.
|
Please don't merge. These tests are failing for both gcc and llvm: |
|
For the group sync one... |
|
Don't think there's actually anything wrong with gdb.dwarf2/ada-thick-pointer.exp. It's just confusion from the CI script due to the group sync testcase errors trumpling over the gdb.dwarf2/ada-thick-pointer.exp testcase output. |
The GDB testsuite uses hipcc for HIP compiler discovery, naming, and documentation. hipcc is deprecated and will be removed at some point, so switch the testsuite over to amdclang++. Compiler discovery (lib/future.exp): - Look for amdclang++ under $ROCM_PATH/lib/llvm/bin instead of hipcc. - Rename HIPCC_FOR_TARGET to HIP_COMPILER_FOR_TARGET (the environment override), and rename gdb_find_hipcc / find_hipcc to gdb_find_hip_compiler / find_hip_compiler. Compile and link flags (lib/gdb.exp, boards/hip.exp): - amdclang++ does not infer the input language from the file extension, so tag each input explicitly: "-x hip" before C/C++/HIP source files and "-x none" before object files, archives and other inputs. This lets a single invocation handle a mix of sources and objects (e.g. set_unbuffered_mode.o and hip-driver.o injected via ldflags). - Pass --hip-link when producing an executable. - Pass --rocm-path only when ROCM_PATH is set in the environment; otherwise defer to amdclang++'s own HIP discovery. - Drop the explicit -O0 from the HIP flags, since amdclang++ already defaults to -O0; an explicit "optimize" option still overrides it. - amdclang++ is clang-based and accepts -fdiagnostics-color, so stop skipping that option for HIP; this keeps color escape sequences out of gdb.log. Testcases: - Rename allow_hipcc_tests to allow_hip_tests in lib/rocm.exp, boards/hip.exp and all gdb.rocm/ and gdb.perf/ testcases. - Replace hipcc's --genco with amdclang++'s --cuda-device-only in gdb.rocm/code-object-load-while-breakpoint-hit.exp and gdb.rocm/snapshot-objfile-on-load.exp, which build a standalone device code object. Documentation (gdb/doc/gdb.texinfo): - Update the example compile command to "amdclang++ -x hip --hip-link ...", keeping the existing note about the equivalent hipcc invocation.
5d59915 to
deab231
Compare
|
Rebased onto the latest What changed since the last push:
I built rocgdb from this branch and ran
|
Motivation
The GDB testsuite uses
hipccfor HIP compiler discovery, naming, anddocumentation.
hipccis deprecated and will be removed at some point, so thisswitches the testsuite over to
amdclang++.What changes
Compiler discovery (
lib/future.exp)amdclang++under$ROCM_PATH/lib/llvm/bininstead ofhipcc.HIPCC_FOR_TARGETtoHIP_COMPILER_FOR_TARGET(the environmentoverride).
gdb_find_hipcc/find_hipcctogdb_find_hip_compiler/find_hip_compiler.Compile and link flags (
lib/gdb.exp,boards/hip.exp)amdclang++does not infer the input language from the file extension, sotag each input explicitly:
-x hipbefore C/C++/HIP sources and-x nonebefore object files/archives. A single invocation can then handle a mix of
sources and objects (e.g.
set_unbuffered_mode.o/hip-driver.oinjectedvia
ldflags).--hip-linkonly when producing an executable.--rocm-pathonly whenROCM_PATHis set; otherwise defer toamdclang++'s own HIP discovery.-O0(amdclang++already defaults to-O0).amdclang++is clang-based and accepts-fdiagnostics-color, so stopskipping it for HIP; this keeps color escape sequences out of
gdb.log.Testcases
allow_hipcc_teststoallow_hip_testsinlib/rocm.exp,boards/hip.exp, and allgdb.rocm/andgdb.perf/testcases.hipcc's--gencowithamdclang++'s--cuda-device-onlyin thetwo code-object tests (
code-object-load-while-breakpoint-hit.exp,snapshot-objfile-on-load.exp).Documentation (
gdb/doc/gdb.texinfo)amdclang++ -x hip --hip-link ...,keeping the note about the equivalent
hipccinvocation.