Skip to content

Conversation

@yasahi-hpc
Copy link
Contributor

May partially resolve #2622
After looking at the bahaviors of older versions, I found that
extent(r) for r > rank should return 1

This PR fixes the behavior.

@yasahi-hpc yasahi-hpc force-pushed the fix-team-gemm-low-rank branch from b49ade1 to 3ee86a4 Compare June 4, 2025 15:49
@yasahi-hpc yasahi-hpc self-assigned this Jun 4, 2025
@lucbv lucbv requested review from lucbv and ndellingwood June 4, 2025 16:16
@lucbv lucbv added bug AT2-CI-APPROVAL Approve CI to run at SNL labels Jun 4, 2025
Copy link
Contributor

@cwpearson cwpearson left a comment

Choose a reason for hiding this comment

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

I don't know the precedence of || and ternary offhand, could you please insert parens (even if not technically necessary)

Copy link
Contributor

@cwpearson cwpearson left a comment

Choose a reason for hiding this comment

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

I'd rather see this conditional nesting inverted with a constexpr check on the rank, but I won't block on it.

Copy link
Contributor

@lucbv lucbv left a comment

Choose a reason for hiding this comment

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

Can we add a small unit-test that exhibit the current issue and shows that the proposed PR fixes it?

@yasahi-hpc
Copy link
Contributor Author

I don't know the precedence of || and ternary offhand, could you please insert parens (even if not technically necessary)

Sure

@yasahi-hpc
Copy link
Contributor Author

Can we add a small unit-test that exhibit the current issue and shows that the proposed PR fixes it?

If this is related, it means that there is a GEMM operation on rank 0 view.
I can add a test for that in #2628

@ndellingwood
Copy link
Contributor

I'm building the Ifpack2 tests to check the impact of this PR, thanks for tracking this down @yasahi-hpc

@ndellingwood
Copy link
Contributor

Unfortunately I still see the failures in Ifpack2_BlockTriDiContainerUnitAndPerfTests_MPI_4 when building Trilinos with this PR, tested in a Cuda build

@yasahi-hpc yasahi-hpc force-pushed the fix-team-gemm-low-rank branch from 3ee86a4 to 285d5ef Compare June 5, 2025 08:19
@yasahi-hpc
Copy link
Contributor Author

Unfortunately I still see the failures in Ifpack2_BlockTriDiContainerUnitAndPerfTests_MPI_4 when building Trilinos with this PR, tested in a Cuda build

Thanks a lot for testing and reporting.
That is a bit unfortunate.
I will continue investigation

@cwpearson cwpearson added AT2-CI-APPROVAL Approve CI to run at SNL and removed AT2-CI-APPROVAL Approve CI to run at SNL labels Jun 5, 2025
Yuuichi Asahi added 3 commits June 13, 2025 16:10
@ndellingwood
Copy link
Contributor

Hi @yasahi-hpc , I retested with 9f1b00a in a Cuda build but I am still seeing failures with the Ifpack2_BlockTriDiContainerUnitAndPerfTests_MPI_4 test

@yasahi-hpc
Copy link
Contributor Author

Hi @yasahi-hpc , I retested with 9f1b00a in a Cuda build but I am still seeing failures with the Ifpack2_BlockTriDiContainerUnitAndPerfTests_MPI_4 test

Thank you for testing.
That is unfortunate.

I will investigate on my side.
First of all, I need to build Trilinos on my environment.

@cwpearson cwpearson added AT2-CI-APPROVAL Approve CI to run at SNL and removed AT2-CI-APPROVAL Approve CI to run at SNL labels Jun 16, 2025
@yasahi-hpc yasahi-hpc mentioned this pull request Oct 17, 2025
@yasahi-hpc
Copy link
Contributor Author

@lucbv Can I close this and start working again #2628 and #2651 ?

@lucbv
Copy link
Contributor

lucbv commented Oct 30, 2025

Yes, I think we can, sorry we took a while to make the final decision on how to move forward.
The code changes in GEMM led to issues with CUDA and HIP so we might want to re-introduce things more slowly, with more smaller PRs...

@yasahi-hpc
Copy link
Contributor Author

Thank you for the information
So, better not to work on #2628 and #2651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT2-CI-APPROVAL Approve CI to run at SNL bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly test failures with Trilinos: various ifpack2 unit test failures

4 participants