Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[External]Add OpenACC Validation and Verification testsuite #38

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

wangxin0321
Copy link

OpenACC is a high-level directive-based parallel programming model that can manage the sophistication of heterogeneity in architectures and abstract it from the users. The portability of the model across CPUs and accelerators has gained the model a wide variety of users. This means it is also crucial to analyze the reliability of the compilers' implementations.

To address this challenge, the OpenACC Validation and Verification team introduces a validation testsuite ,supported by a streamlined infrastructure, to verify the OpenACC implementations across various compilers and system architectures.

@wangxin0321 wangxin0321 reopened this Oct 18, 2023
@wangxin0321 wangxin0321 marked this pull request as draft October 18, 2023 09:48
@wangxin0321 wangxin0321 marked this pull request as ready for review October 18, 2023 09:48
@erichkeane
Copy link

I have no idea how to review this unfortunately! However I believe @sunitachandra showed interest on the Clang RFC, so would have a better idea if this is being done correctly.

@sunitachandra
Copy link

Thanks @erichkeane!
Adding my group's developer, Aaron Jarmusch, to help with the review @ajarmusch

@ajarmusch
Copy link
Contributor

@wangxin0321 Thanks for taking the time to add the OpenACC V&V to Externals

The header files are located in Tests/, so maybe that folder should be included instead of the repo root?

@wangxin0321
Copy link
Author

@ajarmusch
Thank you for your suggestions. I have made the following modifications in response to your suggestions:
1 "set(_includedir "${TEST_SUITE_OpenACCVV_ROOT}/") and target_include_directories(${_name} PRIVATE) "${_includedir}")"(the addition of this header is not required),
At the same time, the following simple modifications were made
1 Change the file name OpenACCV_V.
2 Change the variable names of TEST_SUITE_OFFLOADING_FLAGS and TEST_SUITE_OFFLOADING_LDFLAGS.
3 Remove the "-master"

@ajarmusch
Copy link
Contributor

@wangxin0321 the changes look good

anyone have thoughts? otherwise, LGTM to merge

@erichkeane
Copy link

So how are these configured to run on Clang? Are they using the -fopenacc flag? Do they depend on the _OPENACC macro in some way?

If so on the latter, can we have a build option to make it configurable with the -fexperimental-openacc-macro-override flag as well?

@ajarmusch
Copy link
Contributor

@erichkeane the flags are defined in the cmake command during the test suite build. The README gives an example using -fopenacc

target_link_libraries(${_name} PUBLIC OpenACC::OpenACC_${_lang} m)

# Add -fopenacc to linker command line; for some reason this is not done by target_link_libraries.
target_link_options(${_name} PRIVATE ${OpenACC_${LANG}_FLAGS})

Choose a reason for hiding this comment

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

So this comment isn't accurate, right? It is adding the openacc-language flags, not the -fopenacc? Or where are the OpenACC_C_flags and OpenACC_CXX_flags set?

Copy link
Contributor

Choose a reason for hiding this comment

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

my fault - you were right before

Copy link
Author

Choose a reason for hiding this comment

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

"OpenACC_C_flags "and "OpenACC_CXX_flags" are set in cmake. Check out the official FindOpenACC documentation at the following website: https://cmake.org/cmake/help/latest/module/FindOpenACC.html

But my error, currently find_package(OpenACC), only NVHPC, PGI, GNU and Cray compilers are supported, We cannot use this method to see if the current llvm supports OpenACC. "OpenACC_C_flags "and "OpenACC_CXX_flags" are currently invalid

Copy link
Contributor

Choose a reason for hiding this comment

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

@wangxin0321 Is there a way around? Or we have to make an issue with cmake?

@erichkeane
Copy link

@erichkeane the flags are defined in the cmake command during the test suite build. The README gives an example using -fopenacc

Right, but I'm doubting the usefulness of adding this suite WITHOUT that flag, since that flag is required to enable OpenACC anyway, right? And I was hoping for a more 'builtin' way of using -fexperimental-openacc-macro-override (if the tests use _OpenACC in any way) in the Clang compiler, since we'd like to enable to this to validate our effort.

@ajarmusch
Copy link
Contributor

good point, I looked at the sollve_vv and the OPENMP_C_FLAG flag is set in the OpenMP package. The sollve_vv uses find_package(OpenMP) which is in the root CMakeList.txt. So as long as the idea is to replicate it for OpenACC that should be hardcoded. Would that work?

Also, you are hoping for a builtin option for -fexperimental-openacc-macro-override? If so @wangxin0321 can you add the option for the flag?

@erichkeane
Copy link

good point, I looked at the sollve_vv and the OPENMP_C_FLAG flag is set in the OpenMP package. The sollve_vv uses find_package(OpenMP) which is in the root CMakeList.txt. So as long as the idea is to replicate it for OpenACC that should be hardcoded. Would that work?

Also, you are hoping for a builtin option for -fexperimental-openacc-macro-override? If so @wangxin0321 can you add the option for the flag?

I would love something like that, exposed at a place that is easier to use/pipe in, I suspect this is something I'll use often.

@ajarmusch
Copy link
Contributor

@wangxin0321 Hi, It's been a while since we heard from you. Any progress so far?

@ajarmusch ajarmusch self-assigned this Feb 1, 2024
@ajarmusch
Copy link
Contributor

Hi @wangxin0321, to move this along I am going to help. Since I am unable to make edits to your fork repository. I will create a separate PR and mention this PR and you as well for credit. Reach out as soon as possible

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.

4 participants