Page MenuHomePhabricator

[test-suite] Add HIP Tests to External
ClosedPublic

Authored by ashi1 on Apr 6 2021, 3:36 PM.

Details

Summary

Adding simple HIP Tests, with a simple saxpy.cpp example.
Share the macros using cmake modules with CUDA.

Diff Detail

Repository
rT test-suite

Event Timeline

ashi1 created this revision.Apr 6 2021, 3:36 PM
ashi1 requested review of this revision.Apr 6 2021, 3:36 PM

Thanks for putting this together.
if i knew anything about buildbots i would approve it.
but best to defer to an expert.

tra added a reviewer: beanz.Apr 6 2021, 4:36 PM
tra added a subscriber: beanz.

I've added @beanz as the reviewer who has more cmake know-how.

I'm glad to see HIP support being added to the testsuite. I don't know what your plans are for bringing up an LLVM build/test bot for HIP. If you want, I could probably run a compile-only HIP bot on one of the VMs that run CUDA bots.

cmake/modules/Variant.cmake
18 ↗(On Diff #335672)

This could use further generalization for ".hip" files.

49 ↗(On Diff #335672)

Shared files probably should not rely on variables set somewhere else. Offload should probably be a parameter.
Ditto for Variant* variables. They were OK when everything was in the same file, but less so for something intended for wider use.
They should be specified as a parameter instead.

If we have to keep some parameters as external variables, at the very least such dependencies on external variables must be clearly documented.

ashi1 marked 2 inline comments as done.Apr 9 2021, 9:11 AM
In D99997#2672735, @tra wrote:

I've added @beanz as the reviewer who has more cmake know-how.

I'm glad to see HIP support being added to the testsuite. I don't know what your plans are for bringing up an LLVM build/test bot for HIP. If you want, I could probably run a compile-only HIP bot on one of the VMs that run CUDA bots.

We are planning to add a hip buildbot to test both compilation and execution using various ROCm installations. This allows us to further add tests and experiment with buildbots, and may add OpenMP AMDGPU target.

cmake/modules/Variant.cmake
18 ↗(On Diff #335672)

Thanks, adding .hip.

49 ↗(On Diff #335672)

Thanks, adding these Variant* variables as parameters, and added some comments about the input and outputs of the macro.

ashi1 updated this revision to Diff 336487.Apr 9 2021, 9:12 AM
ashi1 marked 2 inline comments as done.

Revised to Artem's comments, and added empty.hip.

tra accepted this revision.Apr 12 2021, 3:09 PM

LGTM overall.

cmake/modules/Variant.cmake
2 ↗(On Diff #336487)

Maybe we should rename the file to GPUTestVariant to reflect its purpose better.

I'd also add a brief description here describing that it provides common functions/macros for External/{CUDA,HIP}.

49 ↗(On Diff #335672)

Another thing you may consider is whether it would make sense to switch from macros to functions.
I vaguely recall that when I've implemented this originally, cmake had serious trouble dealing with large number of function calls. With thrust tests enabled, there may be thousands of test targets to be created and with functions it took forever. Perhaps cmake can handle it better these days.

Perhaps we could make the macro a private one and instead provide a function using that macro as the public interface.
Once this patch lands, I can check it on thrust tests. If cmake can handle that, then we could fold the macro into the functions. If not, it will be easy enough to switch back to the macro for my own local tests.

This revision is now accepted and ready to land.Apr 12 2021, 3:09 PM
ashi1 marked an inline comment as done.Apr 13 2021, 1:49 PM
ashi1 added inline comments.
cmake/modules/Variant.cmake
2 ↗(On Diff #336487)

Sounds good to me, I will add it.

49 ↗(On Diff #335672)

I think it would be better to introduce functions in a separate commit and have this land with macros first.

Since this method works, should we land as is?

ashi1 updated this revision to Diff 337255.Apr 13 2021, 2:01 PM
ashi1 marked 2 inline comments as done.

Address Artem's comments.

tra accepted this revision.Apr 13 2021, 2:11 PM

LGTM.

cmake/modules/Variant.cmake
49 ↗(On Diff #335672)

I think it would be better to introduce functions in a separate commit and have this land with macros first.

SGTM.

Tagging @jdoerfert as HIP CI may be of interest. I wonder if an openmp directory added here could be persuaded to run on nvptx or amdgpu hardware, based on what happens to be in the bot.

tra added a comment.Apr 22 2021, 1:03 PM

Tagging @jdoerfert as HIP CI may be of interest. I wonder if an openmp directory added here could be persuaded to run on nvptx or amdgpu hardware, based on what happens to be in the bot.

SGTM.

What external dependencies would be needed for OMP?

JonChesterfield added a comment.EditedApr 22 2021, 5:03 PM
In D99997#2709961, @tra wrote:

Tagging @jdoerfert as HIP CI may be of interest. I wonder if an openmp directory added here could be persuaded to run on nvptx or amdgpu hardware, based on what happens to be in the bot.

SGTM.

What external dependencies would be needed for OMP?

Great question! Multiple pieces to the answer so here's a table for the current status.
'hsa' here refers to rocr plus roct which then only needs the linux kernel. Both are easily built via cmake.
'cuda' refers to various pieces of nvidia's toolchain, I'm not clear how the pieces are delimited.

nvptx deviceamdgpu devicenvptx hostamdgpu host
buildnonenonecuda (optional)hsa (required)
executecudanonecudahsa

The host code also uses a few common linux libraries - libelf, libnuma. In the future amdgpu
openmp execution is expected to depend on math functions from rocm-ocml. Doesn't yet.

Build time dependencies can and probably will be reduced to zero required. Runtime can't be
quite so good because there's a kernel level driver in the stack and llvm doesn't bundle libc.