Enable HIP assert in SLIC macros #1498

bmhan12 · 2025-01-29T16:20:22Z

This PR

Enables assert for HIP in the SLIC_ASSERT_* and SLIC_CHECK_* macros.

EDIT:

Fixes the CMAKE_CXX_FLAGS_DEBUG to actually allow assert() for rocm builds.
- Previously, in order to get mesh_tester working with a hip policy in Enable HIP policy for mesh_tester #1151, -DNDEBUG was added to the debug flags.
- This disabled assert() for debug rocm builds.
Updated axom spack recipe and host-configs to reflect CMAKE_CXX_FLAGS_DEBUG change
- Note: %cce debug builds require some optimization (-O1) to compile without cray compiler errors (error is of the form: PLEASE submit a bug report to Cray and include the crash backtrace, preprocessed source, and associated run script. Stack dump: ... ).
Remove checks for NDEBUG in mesh_tester

EDIT 2:

Small change to sbatch option for Gitlab CI ruby jobs: res=ci --> reservation=ci. res is no longer recognized as an option:

sbatch: option '--res=ci' is ambiguous; possibilities: '--reservation' '--resv-ports'

kennyweiss

Thanks @bmhan12

Please update the RELEASE-NOTES to indicate that SLIC_ASSERTs now work w/ hip as well as restrictions (if any)

kennyweiss · 2025-02-03T18:39:01Z

src/axom/slic/interface/slic_macros.hpp

+// Use assert when on device
 #elif defined(AXOM_DEBUG) && defined(AXOM_DEVICE_CODE)
-  #if !defined(__HIP_DEVICE_COMPILE__)
-    #define SLIC_ASSERT(EXP) assert(EXP)
-    #define SLIC_ASSERT_MSG(EXP, msg) assert(EXP)
-    #define SLIC_CHECK(EXP) assert(EXP)
-    #define SLIC_CHECK_MSG(EXP, msg) assert(EXP)
-  #else
-// AXOM_DEBUG is defined so the expression and message cannot be ignored
-// as they probably contain parameters that were decorated with AXOM_UNUSED_PARAM
-// which will require the expression and message to be used to avoid warnings.
-// Here we use them in a way that will not really have a runtime effect.
-// The msg code block may contain << operators so we need a stream-like object
-// but we enclose it in "if(false)" so it never executes.
-namespace axom
-{
-namespace slic
-{
-namespace internal
-{
-class blackhole
-{ };
-// Write an object to a blackhole.
-template <typename T>
-AXOM_HOST_DEVICE inline blackhole &operator<<(blackhole &bh, T)
-{
-  return bh;
-}
-}  // namespace internal
-}  // namespace slic
-}  // namespace axom
-
-    #define SLIC_ASSERT(EXP) ((void)(EXP))
-    #define SLIC_ASSERT_MSG(EXP, msg)            \
-      {                                          \
-        if(false)                                \
-        {                                        \
-          ((void)(EXP));                         \
-          axom::slic::internal::blackhole __oss; \
-          __oss << msg;                          \
-        }                                        \
-      }
-    #define SLIC_CHECK(EXP) ((void)(EXP))
-    #define SLIC_CHECK_MSG(EXP, msg)             \
-      {                                          \
-        if(false)                                \
-        {                                        \
-          ((void)(EXP));                         \
-          axom::slic::internal::blackhole __oss; \
-          __oss << msg;                          \
-        }                                        \
-      }
-  #endif
+  #define SLIC_ASSERT(EXP) assert(EXP)
+  #define SLIC_ASSERT_MSG(EXP, msg) assert(EXP)
+  #define SLIC_CHECK(EXP) assert(EXP)
+  #define SLIC_CHECK_MSG(EXP, msg) assert(EXP)
+


Is there a version of hip/rocm where this starts working and an earlier version where it doesn't work?

kennyweiss · 2025-02-03T18:40:30Z

src/axom/slic/interface/slic_macros.hpp

-      }
-  #endif
+  #define SLIC_ASSERT(EXP) assert(EXP)
+  #define SLIC_ASSERT_MSG(EXP, msg) assert(EXP)


Are there restrictions on msg?
E.g. can only be a simple c-style string and not an iostream expression or fmt formatted string?
Please not these restrictions, if any, here

…recipe accordingly, and undo NDEBUG requirement for mesh_tester hip policy

bmhan12 · 2025-02-05T22:08:02Z

Re-requesting reviews since the PR has expanded to include host-config changes and a Gitlab CI change to fix ruby jobs after the LC update.
I've updated the PR description to reflect the new changes.

rhornung67 · 2025-02-06T16:12:05Z

src/tools/mesh_tester.cpp

@@ -181,7 +181,7 @@ void Input::parse(int argc, char** argv, axom::CLI::App& app)
  #ifdef AXOM_USE_CUDA
  pol_sstr << "\nSet to 'raja_cuda' or 3 to use the RAJA CUDA policy.";
  #endif
-  #if defined(AXOM_USE_HIP) && defined(NDEBUG)
+  #if defined(AXOM_USE_HIP)


This isn't related to your changes, but is there a better way to do this than to have integral values, which IMO are opaque. Maybe, leave those out of the string and just say Set to 'raja_omp' etc.?

rhornung67

Thank you @bmhan12

gunney1

Thank you @bmhan12

white238 · 2025-02-06T21:37:54Z

.gitlab/build_ruby.yml

@@ -7,7 +7,7 @@
 # This is the shared configuration of jobs for ruby
 .on_ruby:
  variables:
-    SCHEDULER_PARAMETERS: "--res=ci --exclusive=user --deadline=now+1hour -N1 -t ${ALLOC_TIME}"
+    SCHEDULER_PARAMETERS: "--reservation=ci --exclusive=user --deadline=now+1hour -N1 -t ${ALLOC_TIME}"


This change is blocking other PRs. Can it be pulled into its own PR? Or can this be PR be merged?

I'll merge it.

bmhan12 added Slic Issues related to Axom's 'slic' component Hip Issues related to Hip labels Jan 29, 2025

bmhan12 force-pushed the feature/han12/hip_assert branch from d65797a to 453111d Compare January 29, 2025 17:40

Enable assert for HIP

d71d652

bmhan12 requested review from white238, cyrush, BradWhitlock, kennyweiss, rhornung67, publixsubfan, gunney1 and Arlie-Capps February 3, 2025 16:01

rhornung67 approved these changes Feb 3, 2025

View reviewed changes

Arlie-Capps approved these changes Feb 3, 2025

View reviewed changes

kennyweiss approved these changes Feb 3, 2025

View reviewed changes

gunney1 approved these changes Feb 3, 2025

View reviewed changes

kennyweiss mentioned this pull request Feb 4, 2025

Initial MIR component #1438

Open

Update notes, add message behavior and versioning details

35b5e4b

bmhan12 force-pushed the feature/han12/hip_assert branch from 6e6a3b2 to 35b5e4b Compare February 4, 2025 17:24

Undo -DNDEBUG flag for debug rocm builds & adjust axom spack package …

f902598

…recipe accordingly, and undo NDEBUG requirement for mesh_tester hip policy

bmhan12 force-pushed the feature/han12/hip_assert branch from 716bde3 to f902598 Compare February 5, 2025 19:40

resolve ambiguous sbatch option --res

07477e0

bmhan12 added Build system Issues related to Axom's build system CI Issues related to continuous integration labels Feb 5, 2025

bmhan12 requested review from Arlie-Capps, gunney1, kennyweiss and rhornung67 February 5, 2025 22:08

rhornung67 reviewed Feb 6, 2025

View reviewed changes

rhornung67 approved these changes Feb 6, 2025

View reviewed changes

gunney1 approved these changes Feb 6, 2025

View reviewed changes

white238 reviewed Feb 6, 2025

View reviewed changes

bmhan12 merged commit 95c0a46 into develop Feb 6, 2025
13 checks passed

bmhan12 deleted the feature/han12/hip_assert branch February 6, 2025 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable HIP assert in SLIC macros #1498

Enable HIP assert in SLIC macros #1498

bmhan12 commented Jan 29, 2025 •

edited

Loading

kennyweiss left a comment

kennyweiss Feb 3, 2025

kennyweiss Feb 3, 2025

bmhan12 commented Feb 5, 2025

rhornung67 Feb 6, 2025

rhornung67 left a comment

gunney1 left a comment

white238 Feb 6, 2025

bmhan12 Feb 6, 2025

Enable HIP assert in SLIC macros #1498

Enable HIP assert in SLIC macros #1498

Conversation

bmhan12 commented Jan 29, 2025 • edited Loading

kennyweiss left a comment

Choose a reason for hiding this comment

kennyweiss Feb 3, 2025

Choose a reason for hiding this comment

kennyweiss Feb 3, 2025

Choose a reason for hiding this comment

bmhan12 commented Feb 5, 2025

rhornung67 Feb 6, 2025

Choose a reason for hiding this comment

rhornung67 left a comment

Choose a reason for hiding this comment

gunney1 left a comment

Choose a reason for hiding this comment

white238 Feb 6, 2025

Choose a reason for hiding this comment

bmhan12 Feb 6, 2025

Choose a reason for hiding this comment

bmhan12 commented Jan 29, 2025 •

edited

Loading