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

InlineVector implementation #3419

Merged
merged 16 commits into from
Jan 16, 2025

Conversation

novakovicdj
Copy link
Contributor

This is PR for initial implementation of InlineVector class from issue #3408, also added some tests for this.

I will do additional performance testing with tensorOp solver structure to check if, and how much, host time is reduced using InlineVector instead of std::vector for lens/strides in TensorDescriptor

src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
std::copy(data.begin(), data.end(), storage.begin());
}

template <typename _InputIterator, typename = std::_RequireInputIter<_InputIterator>>
Copy link
Contributor

Choose a reason for hiding this comment

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

_RequireInputIter is gcc/clang specific.
I guess that part can be omitted at all:
template <typename InputIterator>

@BrianHarrisonAMD
Copy link
Contributor

After develop fix, looks like there is this hip tidy error:

[2024-12-03T18:18:41.307Z] test/gtest/inline_vector_basic_ops.cpp:54:1: warning: syntax error [syntaxError]
[2024-12-03T18:18:41.307Z] TEST(CPU_InlineVectorFindIfAndDistance_NONE, Test)
[2024-12-03T18:18:41.307Z] ^

src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
src/include/miopen/inline_vector.hpp Outdated Show resolved Hide resolved
@CAHEK7
Copy link
Contributor

CAHEK7 commented Jan 7, 2025

@BrianHarrisonAMD @BradPepersAMD @junliume we need this PR for host-side performance improvements (especially around rnn/lstm networks, which is, as I remember, quite critical)
Also it may help to improve testing time a bit.

@BrianHarrisonAMD
Copy link
Contributor

Starting CI for this.

@BrianHarrisonAMD BrianHarrisonAMD merged commit fe1f3ce into ROCm:develop Jan 16, 2025
0 of 3 checks passed
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