Skip to content

Add hip-lang cmake module#2230

Closed
zackgalbreath wants to merge 2 commits intoROCm:mainfrom
zackgalbreath:add_hip_lang_cmake_module
Closed

Add hip-lang cmake module#2230
zackgalbreath wants to merge 2 commits intoROCm:mainfrom
zackgalbreath:add_hip_lang_cmake_module

Conversation

@zackgalbreath
Copy link
Copy Markdown
Contributor

@zackgalbreath zackgalbreath commented Feb 1, 2021

Replaces #2183

Provide CMake hip language configuration information
f9b2aab 
This provides the required information for CMake to bootstrap
HIP language and runtime support. In particular this file
is consumed by CMake during HIP compiler detection to
determine the required flags needed for compilation and
linking.

Initial design was discussed in ROCm#2158
TomSang
TomSang approved these changes Feb 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@TomSang TomSang left a comment

Choose a reason for hiding this comment

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

Looks good to me , but someone else need approve

@TomSang
Copy link
Copy Markdown
Contributor

TomSang commented Feb 1, 2021

Hi @zackgalbreath
Could you change 2020 to 2021 in the following ?
Copyright (c) 2020 - Present Kitware, Inc, and Contributors.

Because the patch is submitted today.

@zackgalbreath zackgalbreath force-pushed the add_hip_lang_cmake_module branch from d319b05 to 6c06d55 Compare February 1, 2021 22:00
@zackgalbreath
Copy link
Copy Markdown
Contributor Author

zackgalbreath commented Feb 1, 2021

Could you change 2020 to 2021 in the following ?
Copyright (c) 2020 - Present Kitware, Inc, and Contributors.

Because the patch is submitted today.

Sure thing. The header is updated per your request.

@TomSang
Copy link
Copy Markdown
Contributor

TomSang commented Feb 2, 2021 •
edited
Loading

Hi @zackgalbreath
Is there any way to install that special cmake?
And can you install samples(with readme) into hip/hip-lang/samples ?
After the patch enters our code base, our QA need test it
Thanks!

@zackgalbreath
Copy link
Copy Markdown
Contributor Author

zackgalbreath commented Feb 2, 2021

Hi @zackgalbreath
Is there any way to install that special cmake?

Certainly!
Our branch that adds HIP language support to CMake currently lives here:
https://gitlab.kitware.com/zackgalbreath/cmake/-/tree/add_hip_language

Documentation on how to build CMake from source can be found here:
https://cmake.org/install/

Our plan is to merge the add_hip_language branch to CMake proper after this HIP PR is merged.

And can you install samples(with readme) into hip/hip-lang/samples ?
After the patch enters our code base, our QA need test it

Sorry, but it's not clear to me what kind of samples you're looking for. Samples of what?

@TomSang
Copy link
Copy Markdown
Contributor

TomSang commented Feb 2, 2021 •
edited
Loading

@zackgalbreath
It's very nice to know where to get that cmake patch !

Robert gave those samples to me for test .
If you have no these samples, please tell me how to send them back to you for submission.

If you have another set of samples, please submit them as well , if possible. I think QA need them.

@TomSang
Copy link
Copy Markdown
Contributor

TomSang commented Feb 3, 2021 •
edited
Loading

Hi @zackgalbreath
Could you change copyright header
Copyright (c) 2021 - Present Kitware, Inc, and Contributors.
into
Copyright (C) 2021 Kitware, Inc. All Rights Reserved.

Thanks!

@zackgalbreath
Copy link
Copy Markdown
Contributor Author

zackgalbreath commented Feb 4, 2021

Hi @zackgalbreath
Could you change copyright header
Copyright (c) 2021 - Present Kitware, Inc, and Contributors.
into
Copyright (C) 2021 Kitware, Inc. All Rights Reserved.

Thanks!

Sure, I can explicitly add "All Rights Reserved" to the copyright header.

Are you also asking me to remove the "and Contributors" part? This is normal for open source projects so we don't hvae to explicitly update the copyright each time somebody new touches the file.

@zackgalbreath zackgalbreath force-pushed the add_hip_lang_cmake_module branch from 6c06d55 to 9d2b236 Compare February 4, 2021 15:42
@aurianer aurianer mentioned this pull request Feb 5, 2021
Merged
@zackgalbreath
Copy link
Copy Markdown
Contributor Author

zackgalbreath commented Feb 9, 2021

I created a Docker image to demonstrate our CMake/HIP integration. Here's how to run it.

  • docker pull zackgalbreath/hip:ubuntu-rocm4.0_repro
  • docker run --device=/dev/kfd --device=/dev/dri --security-opt seccomp=unconfined --group-add video zackgalbreath/hip:ubuntu-rocm4.0_repro

Here's a link to the Dockerfile for this image:
https://gitlab.kitware.com/zackgalbreath/rocm_cmake/-/blob/master/docker/ubuntu/rocm4.0_repro/Dockerfile

And here's the shell script that actually builds it:
https://gitlab.kitware.com/zackgalbreath/rocm_cmake/-/blob/master/docker/build_repro.sh

@MathiasMagnus
Copy link
Copy Markdown
Contributor

MathiasMagnus commented Feb 19, 2021

What is the relation between hip-lang-config.cmake.in and the changes found in the add_hip_language branch of CMake? (I'm just trying to understand how the language support will come together. I'm just closely related to maintaining the CMake scripts of a handful of ROCm libraries and would like to know.)

@zackgalbreath
Copy link
Copy Markdown
Contributor Author

zackgalbreath commented Feb 19, 2021

What is the relation between hip-lang-config.cmake.in and the changes found in the add_hip_language branch of CMake? (I'm just trying to understand how the language support will come together. I'm just closely related to maintaining the CMake scripts of a handful of ROCm libraries and would like to know.)

Good question!

We haven't merged the CMake side of this integration yet, but you can get a sneak peek of it here:
https://gitlab.kitware.com/zackgalbreath/cmake/-/tree/add_hip_language

@MathiasMagnus
Copy link
Copy Markdown
Contributor

MathiasMagnus commented Feb 20, 2021

You can bet I checked that branch and all the unit tests too. 😉 I can't wait to put it to use. I was just curious what the parts external to CMake are doing. Is this script relaying legacy find_package(hip) usage to the new internal machinery?

@MathiasMagnus
Copy link
Copy Markdown
Contributor

MathiasMagnus commented Apr 12, 2021

Any updates on this? Nearly two months of no activity. Have discussions migrated to a different issue/PR? There's only so much the target audience can take without bumping a thread. :)

@pfultz2 pfultz2 mentioned this pull request May 28, 2021
Closed
@mangupta mangupta closed this Aug 5, 2021
@mangupta mangupta reopened this Aug 5, 2021
@mangupta
Copy link
Copy Markdown
Contributor

mangupta commented Aug 5, 2021

Closing all existing pull requests that do not target the develop branch. In case this pull request is still valid, please rebase your changes against develop branch and resubmit.

@mangupta mangupta closed this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomSang TomSang TomSang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zackgalbreath @TomSang @MathiasMagnus @mangupta