Skip to content

Adding back memory check #4106

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

Merged
merged 4 commits into from
Jul 2, 2025
Merged

Adding back memory check #4106

merged 4 commits into from
Jul 2, 2025

Conversation

ivarusic-amd
Copy link
Contributor

Added an option to disable the hipMemGetInfo memory check on Windows when building MGX. By default, the check is enabled, but it can be turned off if desired.

@ivarusic-amd ivarusic-amd requested a review from apwojcik June 30, 2025 13:01
@ivarusic-amd ivarusic-amd requested a review from causten as a code owner June 30, 2025 13:01
@ivarusic-amd ivarusic-amd added the Windows Related changes for Windows Environments label Jun 30, 2025
@pfultz2
Copy link
Collaborator

pfultz2 commented Jun 30, 2025

Whats the reason to disable this memory check?

@ivarusic-amd
Copy link
Contributor Author

We had a similar check on Windows in the past, when we discovered that hipMemGetInfo returns ~70–80% of the available GPU memory. We believe there may be a regression in this function as few models start falling because of memory, and we also need to decide whether we want to enable it.

if(MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK)
message(STATUS "HipMemGetInfo check is disabled")
target_compile_definitions(migraphx_gpu PUBLIC MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dont add a cmake option. I dont think we need this. Just define this to true when windows is used:

#ifdef MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK
#ifdef _WIN32
#define MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK 1
#else
#define MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK 0
#endif
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This macro is intended for internal use only, to quick test specific cases. It will not be activated in production on Windows. The build option allows us to decide whether to include or exclude the memory check code during compilation, we don't want it always ON.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK if its internal then definitely dont add cmake option, just do:

#ifdef MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK
#define MIGRAPHX_DISABLE_AVAILABLE_GPU_MEMORY_CHECK 0
#endif

You can set the option in cmake by passing it to the CMAKE_CXX_FLAGS.

@apwojcik
Copy link
Collaborator

apwojcik commented Jul 2, 2025

@pfultz2 @causten We received an explanation from the HIP team that using hipMemGetInfo in this context is incorrect. The primary intent of the function is to track memory usage trends, rather than determining whether a subsequent hipMalloc call will succeed, especially on APUs. They asked us not to abort applications based solely on a hipMemGetInfo result. Recently, various frameworks have been changing that behaviour, too.
We want to check only if hipMalloc succeeded or failed, and permanently ignore the result of hipMemGetInfo.

@ivarusic-amd ivarusic-amd force-pushed the adding_back_memorycheck branch from 87b2b80 to 32ea66e Compare July 2, 2025 12:07
@apwojcik apwojcik merged commit 57f816f into uai-develop Jul 2, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Related changes for Windows Environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants