Skip to content

Fix for GPU-aware MPI bug #1595

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.

Sign up for GitHub

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

Merged
bmhan12 merged 10 commits into from
Jul 2, 2025
Merged

Fix for GPU-aware MPI bug #1595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
33f480e
Attempt to reproduce gpu aware failures on tioga
bmhan12 Jun 23, 2025
ec33a9e
Extend alloc time
bmhan12 Jun 25, 2025
48d517d
Apply workaround for GPU-aware MPI
bmhan12 Jun 26, 2025
411fd5f
Comment out GPU-aware MPI job for now, add zeroMessage comment per de…
bmhan12 Jun 26, 2025
72e3978
Merge branch 'develop' into ci/han12/gpu_aware
bmhan12 Jun 30, 2025
3870067
Merge branch 'develop' into ci/han12/gpu_aware
bmhan12 Jun 30, 2025
9545cd3
Turn zeroMessage into a function that returns a string; create an all…
bmhan12 Jul 1, 2025
4c4288d
Define zeroMessage in source, point there from header
bmhan12 Jul 1, 2025
d9368f7
Merge branch 'develop' into ci/han12/gpu_aware
bmhan12 Jul 1, 2025
3a46723
Merge branch 'develop' into ci/han12/gpu_aware
bmhan12 Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .gitlab/build_tioga.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ tioga-rocmcc_6_2_1_hip-src:
HOST_CONFIG: "tioga-toss_4_x86_64_ib_cray-${COMPILER}.cmake"
extends: .src_build_on_tioga

# TODO: Translate this job to tuolumne to accomodate node requirements
# tioga-rocmcc_6_3_1_hip-gpu-aware-src:
# variables:
# COMPILER: "rocmcc@6.3.1_hip"
# HOST_CONFIG: "tioga-toss_4_x86_64_ib_cray-${COMPILER}.cmake"
# ALLOC_TIME: "60"
# NUM_NODES: "2"
# extends: .src_build_on_tioga
# before_script:
# - module load cmake/3.21.1
# - export MPICH_GPU_SUPPORT_ENABLED=1

####
# Full Build jobs
tioga-rocmcc_6_2_1_hip-full:
Expand Down
5 changes: 5 additions & 0 deletions src/axom/lumberjack/Message.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ namespace axom
{
namespace lumberjack
{

// Global workaround: https://rzlc.llnl.gov/jira/browse/ELCAP-851
char zeroMessageStorage[] = "0";
const char* const zeroMessage = zeroMessageStorage;

@gberg617 gberg617 Jul 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't you still treating zeroMessage as global data in this case? If I remember correctly, that was the main issue stated later in the ticket.

@bmhan12 bmhan12 Jul 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Aren't you still treating zeroMessage as global data in this case? If I remember correctly, that was the main issue stated later in the ticket.

I think we're still hitting one of the working cases in this scenario: "use global non-constant (for CHAR type this has to be either a char array or a pointer to malloc'ed memory)".

@MrBurmark MrBurmark Jul 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding is that global constants and global non-constants are put in different sections of memory with different permissions. So it may only be the case that global constants are affected but global non-constants are not.


//Getters

std::string Message::text() const { return m_text; }
Expand Down
4 changes: 2 additions & 2 deletions src/axom/lumberjack/Message.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ namespace lumberjack
{
/*!
*****************************************************************************
* \brief Message to indicate no messages need to be sent from child node.
* \brief Message indicating no messages need to be sent from child node.
*****************************************************************************
*/
const char* const zeroMessage = "0";

@kennyweiss kennyweiss Jun 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm understanding the issue properly, would another possible fix be to keep the global variable zeroMessage as is, but copy it to a local variable before each usage?

(Not necessarily requesting any changes, just trying to understand the issue).

@bmhan12 bmhan12 Jun 27, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that should work, and the local variable should be a char array instead of a pointer (Following this bulletpoint from the JIRA ticket: use local variables (although for the CHAR type, local constant needs to be a char array))

@MrBurmark MrBurmark Jul 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to still have this pointer but point it to the storage that was added in Message.cpp with the same name? That way there is only one place that this is stored.

@bmhan12 bmhan12 Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to still have this pointer but point it to the storage that was added in Message.cpp with the same name? That way there is only one place that this is stored.

I'll give this a try.

Based on previous discussion from yesterday, this current iteration has zeroMessage be a function that returns a std::string.

@bmhan12 bmhan12 Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried the following:

// Message.hpp
extern const char * zeroMessage;

---------------------------------------------------------
// Message.cpp
const char * zeroMessage = "0";

The same bug appears in this case. (Code-wise, I'm not sure if this is what you had in mind, if so, let me know!)

@MrBurmark MrBurmark Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking something more like this.

// Message.hpp
extern const char * const zeroMessage;
---------------------------------------------------------
// Message.cpp
char zeroMessageStorage[] = "0";
const char * const zeroMessage = zeroMessageStorage;

@MrBurmark MrBurmark Jul 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could also use a std::array<char, 2> zeroMessage{"0"}; instead of a std::string to make it easy to make a local copy. Then use zeroMessage.data() when you need the pointer.

@bmhan12 bmhan12 Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking something more like this.

Thanks @MrBurmark, that works and is pretty clean!

extern const char* const zeroMessage;

/*!
*****************************************************************************
Expand Down