Skip to content
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

Fix Memory Model and Internal TRSM failure #852

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
+35 −1
Open

Fix Memory Model and Internal TRSM failure #852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c20e3a
fix Memory Model failure with internal trsm
AGonzales-amd Nov 13, 2024
8737cb4
temp. change default to internal blas
AGonzales-amd Nov 13, 2024
61d90cb
revert default internal blas flag
AGonzales-amd Dec 11, 2024
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
4 changes: 4 additions & 0 deletions clients/gtest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ target_compile_definitions(rocsolver-test PRIVATE
ROCSOLVER_CLIENTS_TEST
)

if(ROCSOLVER_USE_INTERNAL_BLAS)
target_compile_definitions(rocsolver-test PRIVATE USE_INTERNAL_GEMM USE_INTERNAL_TRSM)
endif()

add_test(
NAME rocsolver-test
COMMAND rocsolver-test
Expand Down
32 changes: 31 additions & 1 deletion clients/gtest/memory_model_gtest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* **************************************************************************
* Copyright (C) 2020-2023 Advanced Micro Devices, Inc. All rights reserved.
* Copyright (C) 2020-2024 Advanced Micro Devices, Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -204,15 +204,25 @@ TEST_F(checkin_misc_MEMORY_MODEL, user_managed)

// 12. stop query; required size at the end of query (54MB)
rocblas_stop_device_memory_size_query(handle, &size);
#ifndef USE_INTERNAL_TRSM
EXPECT_GT(size, 2000000);
#else
// internal trsm does not use scratch memory
EXPECT_LT(size, 2000000);
#endif
Comment on lines +207 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @AGonzales-amd, those changes fix the failure, but I am afraid that they are not improving the test. The real problem is that this test is making assumptions about the implementation of getrf, and this is brittle: Unless it is absolutely necessary, we should not allow tests to make any assumptions about internal implementations. Implementations can change and render those assumptions redundant (which is exactly what is happening when getrf uses our TRSM).

A more appropriate solution would be to completely remove any calls to getrf that expect it to allocate an implementation dependent amount of memory, and make sure that the test itself sets everything it needs, explicitly.


// 13. device memory size should not change; it should still be 2MB
rocblas_get_device_memory_size(handle, &size);
EXPECT_EQ(size, 2000000);

// 14. When executing getrf, device memory is not enough for execution to success
status = rocsolver_dgetrf_strided_batched(handle, m, n, dA, lda, stA, dP, stP, dinfo, bc);
#ifndef USE_INTERNAL_TRSM
EXPECT_EQ(status, rocblas_status_memory_error);
#else
// internal trsm does not use scratch memory
EXPECT_EQ(status, rocblas_status_success);
#endif

// 15. device memory size should be the same 2MB
rocblas_get_device_memory_size(handle, &size);
Expand Down Expand Up @@ -249,15 +259,25 @@ TEST_F(checkin_misc_MEMORY_MODEL, user_managed)

// 6. stop query
rocblas_stop_device_memory_size_query(handle, &size);
#ifndef USE_INTERNAL_TRSM
EXPECT_GT(size, 2000000);
#else
// internal trsm does not use scratch memory
EXPECT_LT(size, 2000000);
#endif

// 7. device memory size should not change; it should be 2MB
rocblas_get_device_memory_size(handle, &size);
EXPECT_EQ(size, 2000000);

// 8. When executing getrf baseline, device memory is not enough for success
status = rocsolver_dgetrf_strided_batched(handle, m, n, dA, lda, stA, dP, stP, dinfo, bc);
#ifndef USE_INTERNAL_TRSM
EXPECT_EQ(status, rocblas_status_memory_error);
#else
// internal trsm does not use scratch memory
EXPECT_EQ(status, rocblas_status_success);
#endif

// 9. device memory size should be the same 2MB
rocblas_get_device_memory_size(handle, &size);
Expand Down Expand Up @@ -323,15 +343,25 @@ TEST_F(checkin_misc_MEMORY_MODEL, DISABLED_user_owned)

// 10. stop query; required size at the end of query is 54MB
rocblas_stop_device_memory_size_query(handle, &size);
#ifndef USE_INTERNAL_TRSM
EXPECT_GT(size, 2000000);
#else
// internal trsm does not use scratch memory
EXPECT_LT(size, 2000000);
#endif

// 11. device memory size should not change; it should be 2MB
rocblas_get_device_memory_size(handle, &size);
EXPECT_EQ(size, 2000000);

// 12. When executing getrf, device memory is not enough for success
status = rocsolver_dgetrf_strided_batched(handle, m, n, dA, lda, stA, dP, stP, dinfo, bc);
#ifndef USE_INTERNAL_TRSM
EXPECT_EQ(status, rocblas_status_memory_error);
#else
// internal trsm does not use scratch memory
EXPECT_EQ(status, rocblas_status_success);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor questions: Is there a rocsolver function so the user can know say 100MB will be sufficient for the dgetrf rocsolver call for a certain matrix size, or this is just trial-and-error or keep doubling the size until it works? Perhaps when using rocblas managed mode, there is a way to query the max memory pool size used? Perhaps the user can use rocblas managed mode, then later switch to user owned mode (but knowing the max pool size used in rocblas) for higher performance?


// 13. device memory size should be the same 2MB
rocblas_get_device_memory_size(handle, &size);
Expand Down