Skip to content

Fix the GPU Memory Tests#213

Open
MrBurmark wants to merge 8 commits into
mainfrom
bugfix/burmark1/res_mem_test
Open

Fix the GPU Memory Tests#213
MrBurmark wants to merge 8 commits into
mainfrom
bugfix/burmark1/res_mem_test

Conversation

@MrBurmark

@MrBurmark MrBurmark commented Jul 15, 2026

Copy link
Copy Markdown
Member

Fixed the memory tests that were failing for the gpu backends.

A notable breaking change introduced here is no longer treating the unknown memory type as device in allocate for the GPU resources.

Removed tests that attempted to allocate non-zero amounts of unknown memory type memory.
Make the behavior when explicitly attempting to allocate unknown memory type memory more consistent, all gpu backends now throw if you attempt to allocate unknown memory type memory. Note that the cpu backends completely ignore memory type.

Fix an issue where the Hip resource failed to recognize managed memory pointers. This caused failures during deallocation when the memory type had to be looked up.

Fix an issue where a failed allocation could result in calling memset on nullptr in the calloc implementation.

Improve unity in code style of allocation and memory functions across resources.

They are failing as we don't allow deallocating
unknown memory accesses.
Also fix leak in Sycl::allocate
Throw on attempt to allocate unknown with gpu backends,
unless size is 0, previously unknown mapped to device.
Unknown is allowed on the deallcoate side as it will cause
a runtime lookup of the pointer's attributes.

Note that cpu backends ignore MemoryAccess entirely and
Omp only supports device.
Look for the right enum value when checking for managed
pointers in Hip that caused deallocations of managed
pointers to fail if the attributes had to be looked up.
@MrBurmark
MrBurmark requested review from adayton1, bechols97, davidbeckingsale, rchen20, rhornung67 and trws and removed request for rhornung67 July 15, 2026 15:54
@MrBurmark
MrBurmark force-pushed the bugfix/burmark1/res_mem_test branch from aaf9bc8 to c83fe4b Compare July 15, 2026 15:58
previously it would unconditionally call memset which
would presumably segfault
@MrBurmark
MrBurmark force-pushed the bugfix/burmark1/res_mem_test branch from a526bd0 to af0ee60 Compare July 15, 2026 16:02
@rhornung67

Copy link
Copy Markdown
Member

@MrBurmark It looks like the disk quota on azure is being exceeded causing the test failures. Maybe adding cleanup to the Docker file will help; specifically, something like this after the last RUN command with build && ctest
RUN rm -rf /home/camp/workspace/build/* && \ apt-get clean && \ rm -rf /var/lib/apt/lists/*

@MrBurmark

MrBurmark commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@MrBurmark It looks like the disk quota on azure is being exceeded causing the test failures. Maybe adding cleanup to the Docker file will help; specifically, something like this after the last RUN command with build && ctest RUN rm -rf /home/camp/workspace/build/* && \ apt-get clean && \ rm -rf /var/lib/apt/lists/*

I see stuff like setup_env.sh is that important to have as part of this cleanup command?

@MrBurmark

MrBurmark commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@rhornung67 it appears to be complaining about permissions when doing apt-get clean.

@MrBurmark
MrBurmark force-pushed the bugfix/burmark1/res_mem_test branch 3 times, most recently from b141969 to af0ee60 Compare July 16, 2026 17:06
@rhornung67

rhornung67 commented Jul 16, 2026

Copy link
Copy Markdown
Member

@MrBurmark another thought. You could try switching to a the rocm image we use in RAJA testing: https://github.com/llnl/RAJA/blob/develop/Dockerfile#L105 It may be smaller in size.

}
switch (ma) {
case MemoryAccess::Device:
ret = sycl::malloc_device<T>(n, qu);

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 think these function can return a nullptr if the system cannot satisfy the request. Do we want to leave it up to the user to check on that?

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.

maybe a SYCL variant of CAMP_HIP_API_INVOKE_AND_CHECK ?

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