gpu: nvidia: Refactor to native parameters for matmul #2111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShanoToni
requested review from
kala855,
t4c1,
densamoilov,
mgouicem,
dzarukin,
sgeor255 and
dylanangus
github-actions
bot
added
the
platform:gpu-nvidia
ShanoToni
force-pushed
the
cublas_matmul_runtime_refactor
branch
from
6da1758 to
c190dbf
Compare
ShanoToni
changed the title
ShanoToni
force-pushed
the
cublas_matmul_runtime_refactor
branch
from
c190dbf to
2341c03
Compare
densamoilov
reviewed
Sep 27, 2024
| @@ -116,24 +122,25 @@ struct cudnn_matmul_t : cudnn_matmul_base_t { | |||
| } | |||
| return true; | |||
| } | |||
|
|
|||
| std::shared_ptr<cublas_params> params_; | |||
There was a problem hiding this comment.
Is there any reason why we want to have shared ownership over the params?
There was a problem hiding this comment.
It needs to be created in the primitive descriptor and it is used by the implementation, attempting to pass a unique_ptr back and forward from impl to primitive might not be desirable.
densamoilov
reviewed
Sep 27, 2024
densamoilov
reviewed
Sep 27, 2024
densamoilov
reviewed
Sep 27, 2024
ShanoToni
force-pushed
the
cublas_matmul_runtime_refactor
branch
from
2341c03 to
a950e3e
Compare
ShanoToni
force-pushed
the
cublas_matmul_runtime_refactor
branch
from
a950e3e to
b209e92
Compare
densamoilov
approved these changes
Oct 2, 2024
spalicki
approved these changes
Oct 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This refactor is required to address current issues in nvidia backend matmul. Currently the use of runtime dimensions requires native-specific member objects to be initialised, which are specific to the dimensions (Tensor descriptors, matrix layouts). This is handled currently by cleaning up the members and re-initializing on execution. This is not thread safe.
The refactor proposes abstraction of the members to separate structs, which in the non-runtime dimension case would be initialised with the primitive descriptor and passed to the primitive on its respective creation. When not using runtime dimensions the parameters are cleaned up when the primitive is destroyed.
In the runtime dimension case, the parameters are initialised only with the non native specific members. Inside the execute call a copy is created of the new params struct and only then are the native specific structs initialised for the copy. When using runtime dimensions the params are cleaned up at the end of the host_task running the matmul.
Checklist
General
make testandmake test_benchdnn_*) pass locally for each commit?