Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target by aurianer · Pull Request #32901 · spack/spack

aurianer · 2022-09-29T13:57:42Z

Since we use enable_language(HIP) in pika, the architectures were not properly forwarded

albestro · 2022-09-29T16:01:54Z

Isn't this something that we should do in the ROCmPackage build system?

msimberg · 2022-09-29T16:16:47Z

Isn't this something that we should do in the ROCmPackage build system?

It's a good question. I think one problem is that ROCmPackage does not inherit from CMakePackage (or AutotoolsPackage etc.), so it can't automatically set CMAKE_HIP_ARCHITECTURES (though I don't know python very well so I may be missing something). I suppose one alternative would be a cmake_flags method like hip_flags (

spack/lib/spack/spack/build_systems/rocm.py

Lines 136 to 139 in 679cd4b

    
           @staticmethod 
        
           def hip_flags(amdgpu_target): 
        
               archs = ",".join(amdgpu_target) 
        
               return "--amdgpu-target={0}".format(archs)

) that at least centralizes the definitions. However, that sounds like something that should be done separately from this. There are already a few other packages setting CMAKE_HIP_ARCHITECTURES like here. The same concern applies to CMAKE_CUDA_ARCHITECTURES by the way.

albestro · 2022-09-29T17:04:34Z

It sounds reasonable, good point. I agree we can think about it in another PR and eventually “fix” later the packages. 😉

tldahlgren

Let me know if you want this PR merged as-is.

tldahlgren · 2022-09-29T20:16:38Z

var/spack/repos/builtin/packages/pika/package.py

            args += [self.define("CMAKE_CXX_COMPILER", self.spec["hip"].hipcc)]
            if self.spec.satisfies("^[email protected]:3.21.2"):
                args += [self.define("__skip_rocmclang", True)]
+        if "@0.8: +rocm" in self.spec:


We generally recommend using self.spec.satisfies() when checking version ranges (https://spack.readthedocs.io/en/latest/packaging_guide.html#testing-spec-constraints), which this includes.

Suggested change

if "@0.8: +rocm" in self.spec:

if self.spec.satisfies("@0.8: +rocm"):

Thanks for pointing that out! The if "@:0.7 ..." line earlier should presumably also be changed in that case?

To be honest I can't quite grok what the difference between the two is based on the documentation you linked. It says "The difference between the two functions is that satisfies() tests whether spec constraints overlap at all, while in tests whether a spec or any of its dependencies satisfy the provided spec". "spec constraints overlap at all" and "satisfy the provided spec" sounds similar-ish. The only meaningful distinction I see there is that the latter says "a spec or any of its dependencies", but I don't quite understand the relevance of it. Is there another place explaining the difference in more detail? More importantly, is there any reason to prefer in (besides that it reads nicely)?

I just updated the package to use satisfies but I'm still curious about the difference between the two :)

)

…(py-dendropy, py-phylophlan, py-pkgconfig) (#32936) * added metaphlan v4, cleaned up phylophlan * added iqtree2 * fixed phylophlan, builds now * changed config.yaml to default * fixed style * py-jsonschema: add 4.16.0 and new package py-hatch-fancy-pypi-readme (#32929) * acfl: add v22.1 (#32915) Co-authored-by: Annop Wongwathanarat <[email protected]> * Fixup errors introduced by Clingo Pr: (#32905) * re2c depends on cmake on Windows * Winbison properly added to bootstrap package search list * Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target (#32901) * libtiff: default to +zlib+jpeg (#32945) * octave: add version 7.2.0 (#32943) * simgrid new releases (#32920) * [rocksdb] Added rtti variant (#32918) * rvs binary path updated for 5.2 rocm release (#32892) * Add checksum for py-pytest-runner 6.0.0 (#32957) * py-einops: add v0.5.0 (#32959) * Replace repo with the NVIDIA one (#32951) * Add checksum for py-tomli 2.0.1 (#32949) * QMCPACK: add @3.15.0 (#32931) * Tidied up configure arguments to use special spack autotools features. (#32930) * casper: old domain fell off, adding github repo (#32928) * unifyfs: pin mercury version; add boost variant (#32911) Mercury has a new version (v2.2) releasing soon that UnifyFS does not build with and hasn't been tested with. This pins UnifyFS to the last version of Mercury used/tested. Add a variant to avoid building/using boost Append -std=gnu99 to cflags if building with gcc@4. Needed for mochi-margo to compile * trilinos: constrain superlu-dist version (#32889) * trilinos: constrain superlu-dist version for 13.x * syntax * FEniCSx: Updates for 0.5.1 (#32665) * Updates for DOLFINx 0.5.1 and associated packages * xtensor needed on anything less than main * Switch back to Python 3.7 minimum. * Might be good to point out in our README how to fix Python version? * Fix basix, xtensor dep * Add numba feature * Fix checksum * Make slepc optional Co-authored-by: Tamara Dahlgren <[email protected]> * simgrid: add variant and remove flag (#32797) * simgrid: remove std c++11 flag * simgrid: add msg variant * Axom: bring in changes from axom repo (#32643) * bring in changes from axom repo Co-authored-by: white238 <[email protected]> Co-authored-by: Tamara Dahlgren <[email protected]> * Add checksum for py-pyparsing 3.0.9 (#32952) * rdma-core: fix syntax for external discoverability (#32962) * Add checksum for py-flatbuffers 2.0.7 (#32955) * amrex: add v22.10 (#32966) * Remove CMakePackage.define alias from most packages (#32950) * Bug fix for `ca-certificates-mozilla/package.py` to enable `spack install --source` (#32953) * made suggested changes to iqtree2, py-dendropy, py-metaphlan, and py-pkgconfig. Poetry install still broken * reverted py-pkgconfig deps to poetry-core * made iqtree2 less dedundant, changes to py-dendropy and py-pkgconfig deps Co-authored-by: Manuela Kuhn <[email protected]> Co-authored-by: Annop Wongwathanarat <[email protected]> Co-authored-by: Annop Wongwathanarat <[email protected]> Co-authored-by: John W. Parent <[email protected]> Co-authored-by: Auriane R <[email protected]> Co-authored-by: Adam J. Stewart <[email protected]> Co-authored-by: Kai Torben Ohlhus <[email protected]> Co-authored-by: Vinícius <[email protected]> Co-authored-by: Matthieu Dorier <[email protected]> Co-authored-by: renjithravindrankannath <[email protected]> Co-authored-by: iarspider <[email protected]> Co-authored-by: Paul R. C. Kent <[email protected]> Co-authored-by: Brian Van Essen <[email protected]> Co-authored-by: snehring <[email protected]> Co-authored-by: Cameron Stanavige <[email protected]> Co-authored-by: Cody Balos <[email protected]> Co-authored-by: Jack S. Hale <[email protected]> Co-authored-by: Tamara Dahlgren <[email protected]> Co-authored-by: Lucas Nesi <[email protected]> Co-authored-by: Chris White <[email protected]> Co-authored-by: white238 <[email protected]> Co-authored-by: Martin Pokorny <[email protected]> Co-authored-by: Weiqun Zhang <[email protected]> Co-authored-by: Massimiliano Culpo <[email protected]> Co-authored-by: Dom Heinzeller <[email protected]>

Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target

9147c60

spackbot-app bot added the update-package label Sep 29, 2022

spackbot-app bot requested review from albestro, msimberg and teonnik September 29, 2022 13:57

msimberg previously approved these changes Sep 29, 2022

View reviewed changes

Format with black

f1e719f

aurianer dismissed msimberg’s stale review via f1e719f September 29, 2022 14:12

msimberg previously approved these changes Sep 29, 2022

View reviewed changes

tldahlgren assigned msimberg Sep 29, 2022

tldahlgren previously approved these changes Sep 29, 2022

View reviewed changes

tldahlgren self-assigned this Sep 29, 2022

teonnik previously approved these changes Oct 1, 2022

View reviewed changes

Update to spec.satisfies

7772577

aurianer dismissed stale reviews from teonnik, tldahlgren, and msimberg via 7772577 October 3, 2022 07:04

msimberg approved these changes Oct 3, 2022

View reviewed changes

haampie approved these changes Oct 3, 2022

View reviewed changes

haampie merged commit 7ee8fd5 into spack:develop Oct 3, 2022

luke-dt pushed a commit to dantaslab/spack that referenced this pull request Oct 4, 2022

Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target (spack#32901

af88971

)

aurianer deleted the add_cmake_hip_architectures branch July 4, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target#32901

Set CMAKE_HIP_ARCHITECTURES with the value of amdgpu_target#32901
haampie merged 3 commits intospack:developfrom
aurianer:add_cmake_hip_architectures

aurianer commented Sep 29, 2022

Uh oh!

albestro commented Sep 29, 2022

Uh oh!

msimberg commented Sep 29, 2022

Uh oh!

albestro commented Sep 29, 2022

Uh oh!

tldahlgren left a comment

Uh oh!

tldahlgren Sep 29, 2022

Uh oh!

msimberg Sep 30, 2022

Uh oh!

aurianer Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	if "@0.8: +rocm" in self.spec:
	if self.spec.satisfies("@0.8: +rocm"):

Conversation

aurianer commented Sep 29, 2022

Uh oh!

albestro commented Sep 29, 2022

Uh oh!

msimberg commented Sep 29, 2022

Uh oh!

albestro commented Sep 29, 2022

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

tldahlgren Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

msimberg Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

aurianer Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants