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.

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

[MKL][ROCBLAS] support the omatcopy and omatadd functions in row and column majors #338

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

jinz2014
Copy link
Contributor

Thank you for your review.

@muhammad-tanvir-1211
Copy link
Contributor

Hi @jinz2014
Similar to #333, do you mind attaching the test log and confirm if clang-format has indeed been applied to this patch? Thanks

@jinz2014
Copy link
Contributor Author

jinz2014 commented Aug 4, 2023

I will need to update this PR. Thanks for your reminder.

@jinz2014 jinz2014 changed the title [MKL][ROCBLAS] add the support of omatcopy in row-major layout [MKL][ROCBLAS] support the omatcopy and omatadd functions in row and column majors Aug 22, 2023
@jinz2014
Copy link
Contributor Author

@muhammad-tanvir-1211
I tried to update the files. The build is successful, but there are failures when running the ctests. Could you please run the ctests and let me know if they work ? Thank you.

@muhammad-tanvir-1211
Copy link
Contributor

Hi @jinz2014, apologies for the delay. I was able to recreate the test failures locally. I have suggested the fix for this in the code. Thanks.

jinz2014 and others added 8 commits September 24, 2023 10:44
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
@jinz2014
Copy link
Contributor Author

@muhammad-tanvir-1211 The codes were updated with your suggestions. Thanks.

@muhammad-tanvir-1211
Copy link
Contributor

Great, could you run the tests locally to see if they are passing on your end as well, and attach the log here?

@jinz2014
Copy link
Contributor Author

I have a question. Did you run the tests successfully on your end with your suggested changes ?

@muhammad-tanvir-1211
Copy link
Contributor

Yes, I did.

@jinz2014
Copy link
Contributor Author

There are segfaults when running 'ctest'.

Note: Google Test filter = OmataddTestSuite/OmataddTests.RealSinglePrecision/Column_Major_AMD_Instinct_MI100
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from OmataddTestSuite/OmataddTests
[ RUN      ] OmataddTestSuite/OmataddTests.RealSinglePrecision/Column_Major_AMD_Instinct_MI100
[New Thread 0x7fffea9ff640 (LWP 368626)]
[       OK ] OmataddTestSuite/OmataddTests.RealSinglePrecision/Column_Major_AMD_Instinct_MI100 (563 ms)
[----------] 1 test from OmataddTestSuite/OmataddTests (563 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (563 ms total)
[  PASSED  ] 1 test.

Thread 4 "test_main_blas_" received signal SIGSEGV, Segmentation fault.
[Switching to thread 4 (Thread 0x7fffea9ff640 (LWP 368626))]
0x00007ffff2c30bfd in ?? () from /opt/rocm/hip/lib/libamdhip64.so.5
(gdb) bt
#0  0x00007ffff2c30bfd in ?? () from /opt/rocm/hip/lib/libamdhip64.so.5
#1  0x00007ffff2c3119a in ?? () from /opt/rocm/hip/lib/libamdhip64.so.5
#2  0x00007ffff2a7bd6f in ?? () from /opt/rocm/hip/lib/libamdhip64.so.5
#3  0x00007ffff2a81eb7 in hipFree () from /opt/rocm/hip/lib/libamdhip64.so.5
#4  0x00007ffdd2cf0320 in _rocblas_handle::~_rocblas_handle() () from /opt/rocm-5.4.0/lib/librocblas.so.0
#5  0x00007ffdd2cf1f96 in rocblas_destroy_handle () from /opt/rocm-5.4.0/lib/librocblas.so.0
#6  0x00007fffebd413a8 in oneapi::mkl::blas::rocblas::rocblas_handle_container<_pi_context*>::~rocblas_handle_container() ()
   from /noback/5zj/codeplay/oneMKL/build-rocblas2/lib/libonemkl_blas_rocblas.so
#7  0x00007ffff4595d9f in __GI___call_tls_dtors () at ./stdlib/cxa_thread_atexit_impl.c:159
#8  0x00007ffff45e49c5 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:450
#9  0x00007ffff4676a00 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Could you please paste the test log ?

@muhammad-tanvir-1211
Copy link
Contributor

Hi @jinz2014,
Please find attached the changes I made to rocblas_extensions file and the relevant test logs. Thanks.

rocblas_changes.log
omatcopy_omatadd.log

@jinz2014
Copy link
Contributor Author

A few tests are skipped in the log. I am not sure which tests should be skipped. Can you please have another reviewer evaluate the PR ? Thanks.

@muhammad-tanvir-1211
Copy link
Contributor

Maybe @andrewtbarker can help?

@andrewtbarker
Copy link
Contributor

/intelci: run

@andrewtbarker
Copy link
Contributor

It looks like it is skipping the batch tests, but the added omatcopy and omatadd tests are passing. That's fine, we can leave the batch versions unimplemented for now and put them in a future PR if necessary.

@jinz2014
Copy link
Contributor Author

Thank you for your comments

@andrewtbarker
Copy link
Contributor

The CI doesn't like the way the edits are formatted. Can you check clang-format again and make sure you're using the "correct" version 9.0.0 (yes, that's an old version, but it's the standard for this repo).

Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Thanks for the additions!

@jinz2014
Copy link
Contributor Author

jinz2014 commented Oct 2, 2023

Thank you for your suggestion.

Co-authored-by: Muhammad Tanvir <84532306+muhammad-tanvir-1211@users.noreply.github.com>
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