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

[WIP] Update and standardize implementation of packages, in sync with spack update #480

Merged
merged 55 commits into from
Nov 4, 2024

Conversation

adrienbernede
Copy link
Member

@adrienbernede adrienbernede commented Sep 25, 2024

Summary

This PR :

  • migrates CARE and Caliper to CachedCMakePackage, reducing the gap with implementations found in llnl/radiuss-spack-configs.
  • improves coherency in version constraints across RADIUSS packages.
  • updates Spack.
  • updates RAJA to latest develop.
  • NOTE: This PR contains changes to kernels with reductions to update to the new val-op interface which is needed to build with RAJA develop.

👀 Deserves some consideration:

  • The spec added in CI for sycl on corona had +openmp. I had to turn openmp support off due to a build error. Openmp support is turned off for the same job in RAJA CI. Then I had to turn benchmarks off. Now it's something else. Allowing the job to fail for now.

⚠️ TODO Before Merge:

@rhornung67
Copy link
Member

@adrienbernede I don't understand why SYCL CI is failing. I can build and run the tests manually on corona using the scripts/lc-builds/corona_sycl.sh script. Please note that I fixed a couple of issues in this PR: #481. Those changes were necessary for me to build and run successfully.

The build script mentioned here may be helpful for you to get the CI SYCL build correct.

@rhornung67
Copy link
Member

Also, OpenMP can be enabled for the SYCL checks.

@adrienbernede
Copy link
Member Author

@rhornung67
I had turned off OpenMP because of this:

CMake Error at /usr/tce/backend/installations/linux-rhel8-x86_64/gcc-10.3.1/cmake-3.23.1-mdfqd2l7c33zg7xcvqizwz25vqmp7jfw/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
Call Stack (most recent call first):
  /usr/tce/backend/installations/linux-rhel8-x86_64/gcc-10.3.1/cmake-3.23.1-mdfqd2l7c33zg7xcvqizwz25vqmp7jfw/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  /usr/tce/backend/installations/linux-rhel8-x86_64/gcc-10.3.1/cmake-3.23.1-mdfqd2l7c33zg7xcvqizwz25vqmp7jfw/share/cmake-3.23/Modules/FindOpenMP.cmake:545 (find_package_handle_standard_args)
  blt/cmake/thirdparty/BLTSetupOpenMP.cmake:11 (find_package)
  blt/cmake/BLTSetupTargets.cmake:86 (include)
  blt/cmake/SetupThirdParty.cmake:6 (include)
  blt/SetupBLT.cmake:129 (include)
  CMakeLists.txt:28 (include)

I'll try with your fix.

@rhornung67 rhornung67 dismissed their stale review October 22, 2024 21:47

Issue needs to be fixed

@rhornung67
Copy link
Member

@adrienbernede I pushed a change to the raja-perf spack package that may fix the SYCL CI issue. If this works, we will need to merge a sequence of PRs, in order: first pull radiuss-spack-configs branch with Spack package change, second pull updated radiuss-spack-configs main into RAJA develop, third pull new RAJA develop into this PR branch. Then, we should be able to merge this (fingers crossed).

@rhornung67
Copy link
Member

@adrienbernede I found and fixed the issue. See this commit: 01bf5b2

@adayton1 adayton1 changed the title [WIP] Update and sandardize implementation of packages, in sync with spack update [WIP] Update and standardize implementation of packages, in sync with spack update Oct 29, 2024
@rhornung67 rhornung67 requested a review from rchen20 October 30, 2024 16:15
@rhornung67
Copy link
Member

@rchen20 @MrBurmark This will be merged soon after a couple of other PRs are merged on other projects so that this can point at develop/main branches of RAJA and radiuss-spack-configs. To do these updates, we pulled the reduction kernels changes to the new val-op interface into this PR. Please review those changes in this PR before this is merged. Thank you.

Comment on lines 174 to 181
RAJA::expt::ValLoc<Real_type, RAJA::Index_type> tminloc(m_xmin_init,
m_initloc);

RAJA::forall<exec_policy>( res,
RAJA::RangeSegment(ibegin, iend),
RAJA::expt::Reduce<RAJA::operators::minimum>(&tloc),
[=] __device__ (Index_type i, VL_TYPE& loc) {
loc.min(x[i], i);
RAJA::expt::Reduce<RAJA::operators::minimum>(&tminloc),
[=] __device__ (Index_type i,
RAJA::expt::ValLocOp<Real_type, Index_type,
Copy link
Member

Choose a reason for hiding this comment

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

The ValLocOp Index_type should match the above ValLoc's RAJA::Index_type. I can't quite tell if they match, but we should check. It's probably working due to type erasure in the internals.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch -- thanks!. That's a cut-and-paste error. The types are the same and the types in RAJAPerf are aliases to the RAJA types. I pushed a change plus some others for code consistency.

loc.min(x[i], i);
RAJA::expt::Reduce<RAJA::operators::minimum>(&tminloc),
[=] (Index_type i,
RAJA::expt::ValLocOp<Real_type, Index_type,
Copy link
Member

Choose a reason for hiding this comment

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

Should check matching Index_type here too.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed.

loc.min(x[i], i);
RAJA::expt::Reduce<RAJA::operators::minimum>(&tminloc),
[=](Index_type i,
RAJA::expt::ValLocOp<Real_type, Index_type,
Copy link
Member

Choose a reason for hiding this comment

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

Should check matching Index_type here too.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@rchen20 rchen20 left a comment

Choose a reason for hiding this comment

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

LGTM

@rhornung67
Copy link
Member

rhornung67 commented Oct 31, 2024

@adrienbernede This PR is synced up with latest radiuss-spack-configs main and RAJA develop. All checks passed. You may have the honor of merging.

@adrienbernede adrienbernede merged commit 964e21b into develop Nov 4, 2024
40 checks passed
@adrienbernede adrienbernede deleted the woptim/spack-update branch November 4, 2024 13:55
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.

3 participants