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

Jump to bottom

RNN Dynamic algo single stream #3406

Merged
merged 23 commits into from
Jan 7, 2025
Merged

RNN Dynamic algo single stream #3406

merged 23 commits into from
Jan 7, 2025

Conversation

shurale-nkn
Copy link
Contributor

Added new algo type (miopenRNNroundedDynamic) for LSTM. Should produce less unique requests to rocblass.
Prerequisite for multi-stream version.
Prerequisite for native support of the padded_seq layout.

CAHEK7
CAHEK7 approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@CAHEK7 CAHEK7 left a comment

Choose a reason for hiding this comment

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

There are some minor recommendations.
Unfortunately I can't go deep into the code, since I'm not familiar with the algorithm itself.

src/include/miopen/rnn/tmp_buffer_utils.hpp Outdated Show resolved Hide resolved
src/rnn/Solutions/bwd_s_stream.cpp Outdated Show resolved Hide resolved
src/include/miopen/rnn/algorithms/default_algo_utils.hpp Show resolved Hide resolved
@shurale-nkn
Copy link
Contributor Author

Hi @junliume @BrianHarrisonAMD @BradPepersAMD, when we can merge this ? Thanks

BradPepersAMD
BradPepersAMD reviewed Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@BradPepersAMD BradPepersAMD left a comment

Choose a reason for hiding this comment

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

I'm not seeing any unit tests that seem to really validate the new RNN algorithm from a correctness perspective. Is there something I'm missing?

@shurale-nkn
Copy link
Contributor Author

I'm not seeing any unit tests that seem to really validate the new RNN algorithm from a correctness perspective. Is there something I'm missing?

For the user, the result of this algorithm is identical to the standard one. Therefore, the general test logic extended by testing with this algorithm is sufficient. Partially, this was the reason for adding a separate type.

@shurale-nkn shurale-nkn merged commit 370e378 into develop Jan 7, 2025
68 of 144 checks passed
@shurale-nkn shurale-nkn deleted the dlstm_fwd_bwd branch January 7, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BradPepersAMD BradPepersAMD BradPepersAMD left review comments

@CAHEK7 CAHEK7 CAHEK7 approved these changes

@junliume junliume junliume approved these changes

@BrianHarrisonAMD BrianHarrisonAMD Awaiting requested review from BrianHarrisonAMD BrianHarrisonAMD is a code owner

Assignees
No one assigned
Labels
complexity_middle TESTING_CI_PASSED urgency_blocker urgency_high
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shurale-nkn @CAHEK7 @junliume @BradPepersAMD