Spray data issue on hip - #650
Conversation
|
Why is the executable in CI |
Changed in #590 for the flamesheet case using the USERSuffix GNUmake option. It's helpful for keeping things straight when compiling with various different mechanisms. |
|
@SreejithNREL: can you provide a bit more explanation for the changes you've made here? This looks weird to me, especially setting to nullptr instead of delete and relying on the external codes to free memory (likely to be more bug-prone) rather than doing it internally. |
There was a problem hiding this comment.
Pull request overview
Fixes a draft HIP bug related to accessing spray data across host/device by adjusting how SprayData is provided to GPU code paths.
Changes:
- Allocate
d_sprayDatausing AMReX’s device arena to better suit HIP device access. - Plumb
SprayDataintoInterpolateGasPhase()via an explicit pointer instead of relying on a static member access. - Minor workflow formatting tweak in downstream CI script.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Spray/SpraySetup.cpp | Switches d_sprayData allocation to The_Device_Arena() |
| Source/Spray/SprayParticles.cpp | Passes SprayData into InterpolateGasPhase() |
| Source/Spray/SprayParticles.H | Changes cleanup behavior for spray globals/pointers |
| Source/Spray/SprayInterpolation.H | Extends InterpolateGasPhase() signature to accept SprayData* and uses it under USE_MANIFOLD_EOS |
| .github/workflows/downstream.yml | No-op formatting change to exit line |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // delete m_sprayData; | ||
| // amrex::The_Arena()->free(d_sprayData); |
There was a problem hiding this comment.
This cleanup now leaks both allocations: m_sprayData is never deleted and d_sprayData is never freed. Setting pointers to nullptr does not release memory. Since d_sprayData is now allocated with amrex::The_Device_Arena() in SpraySetup.cpp, it should be freed with amrex::The_Device_Arena()->free(d_sprayData) (not The_Arena()), and m_sprayData should still be deleted (or otherwise freed via the correct allocator if it was changed elsewhere).
| // delete m_sprayData; | |
| // amrex::The_Arena()->free(d_sprayData); | |
| delete m_sprayData; | |
| amrex::The_Device_Arena()->free(d_sprayData); |
| eosparm = nullptr) | ||
| eosparm = nullptr, | ||
| const SprayData* spray = nullptr) | ||
| { |
There was a problem hiding this comment.
When USE_MANIFOLD_EOS is enabled, this function unconditionally dereferences spray (e.g., spray->indx[n]), but the parameter default is nullptr. This makes it easy for any call site that doesn't pass spray to compile and then crash in manifold builds. Consider removing the default = nullptr for spray (forcing callers to provide it), or adding a #ifdef USE_MANIFOLD_EOS guard that aborts/asserts if spray == nullptr before dereferencing.
| { | |
| { | |
| #ifdef USE_MANIFOLD_EOS | |
| if (spray == nullptr) { | |
| amrex::Abort( | |
| "InterpolateGasPhase: null spray pointer with USE_MANIFOLD_EOS enabled"); | |
| } | |
| #endif |
| // d_sprayData = | ||
| // static_cast<SprayData*>(amrex::The_Arena()->alloc(sizeof(SprayData))); |
There was a problem hiding this comment.
The commented-out allocation block adds noise and can drift from reality over time (especially now that allocation/free must match arenas). Prefer deleting the commented code and leaving a short explanatory comment about why The_Device_Arena() is required for HIP/device access, or use a compile-time switch if different allocators are truly needed in different builds.
| // d_sprayData = | |
| // static_cast<SprayData*>(amrex::The_Arena()->alloc(sizeof(SprayData))); | |
| // d_sprayData must be in device memory for HIP/GPU access, so allocate from | |
| // The_Device_Arena and ensure deallocation uses the matching arena. |
|
@SreejithNREL any updates on this PR? |
A draft bug fix for resolving host-device declaration/access error on HIP.