Skip to content

Hip wrapper - #20

Draft
scheibelp wants to merge 5 commits into
spack:mainfrom
scheibelp:hip-wrapper
Draft

Hip wrapper#20
scheibelp wants to merge 5 commits into
spack:mainfrom
scheibelp:hip-wrapper

Conversation

@scheibelp

@scheibelp scheibelp commented Jul 9, 2026

Copy link
Copy Markdown
Member

Used by spack/spack-packages#4411

Pipelines there fully passed with latest commit (d6cc6b2) 2b8ef4e (additional commit after that is for cleanup)

@scheibelp
scheibelp marked this pull request as draft July 9, 2026 23:57
@scheibelp

Copy link
Copy Markdown
Member Author

Question: should cxxflags attribute from compiler definitions (in packages.yaml) be added when detected language is HIP? or should there be a separate hipflags entry that adds flags to hip invocations. See https://github.com/spack/compiler-wrapper/blob/main/cc.sh#L789

Also, note that if it were processed like cxxflags: it would not work to add -x hip (because that gets placed after the source file, which is apparently important to amdclang++).

@haampie

haampie commented Jul 21, 2026

Copy link
Copy Markdown
Member

Did you check what CMake does? They implemented it twice, so presumably the second time they learned from mistakes from the past. See https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html#variable:CMAKE_%3CLANG%3E_FLAGS and various other pages, or sources.

Edit: I did that:

  • CMake passes -x hip, it's not extension based; we should do the same, otherwise the compiler wrapper may at some point need compiler+version specific parsing in case the real compiler changes behavior.
  • CMAKE_HIP_FLAGS_INIT is dervied from HIPFLAGS
  • CXXFLAGS is unrelated to HIPFLAGS.

That makes sense cause it's a different language.

This is the case for ~5 years, so I would say let's do -x hip only and fix upstream packages if needed, or deprecate them if too difficult.

@scheibelp

Copy link
Copy Markdown
Member Author

Commit ed41d0c depends on spack/spack#52826. This PR can be merged first (hipflags isn't useful until this, spack/spack-packages#4411, and spack/spack#52826 merge)

@haampie

haampie commented Aug 12, 2026

Copy link
Copy Markdown
Member

You should add tests.

Comment thread cc.sh
;;
hip|spackhip)
command="$SPACK_HIPCXX"
vdep="spack-hip"

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.

Suggested change
vdep="spack-hip"
vdep="hip-lang"

Comment thread cc.sh
case "$arg" in
-x|--language)
_lang="$1"
shift ;;

@haampie haampie Aug 12, 2026

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 breaks ld ... -x on dash where you can't shift an empty argument list

$ man ld
...

       -x
       --discard-all
           Delete all local symbols.

Maybe run it only when wrapping a compiler?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants