Skip to content

[NFC] Remove MatrixUse::Unnecessary#7335

Merged
steffenlarsen merged 3 commits into
intel:syclfrom
MrSidims:remove-matrix-use-unnecessary
Nov 17, 2022
Merged

[NFC] Remove MatrixUse::Unnecessary#7335
steffenlarsen merged 3 commits into
intel:syclfrom
MrSidims:remove-matrix-use-unnecessary

Conversation

@MrSidims

@MrSidims MrSidims commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

It was previously introduced for backwards compatibility with legacy (without Use matrix parameter) API. It is actually not needed.

Signed-off-by: Sidorov, Dmitry dmitry.sidorov@intel.com

It was previously introduced for backwards compatibility with
legacy (without Use matrix paramter) API. It is actually not needed.

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
@MrSidims MrSidims requested a review from a team as a code owner November 9, 2022 16:29
@MrSidims MrSidims requested a review from romanovvlad November 9, 2022 16:29
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
template <typename T, std::size_t R, std::size_t C,
__spv::MatrixUse U = __spv::MatrixUse::Unnecessary,
template <typename T, std::size_t R, std::size_t C, __spv::MatrixUse U,
__spv::MatrixLayout L = __spv::MatrixLayout::RowMajor,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we are making the change to dynamic in this PR right?
If yes, the default for layout should be dynamic

@MrSidims MrSidims Nov 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This PR was meant by me to just remove MatrixUse::Unnecessary. Adding dynamic requires a change in layout enum guarding it with if MATRIX_VERSION - and AFAIK Bing is doing this work.

@@ -39,9 +39,7 @@ SPV_MATRIX_LAYOUT_TRAITS(layout::packed_a, __spv::MatrixLayout::PackedA)
SPV_MATRIX_LAYOUT_TRAITS(layout::packed_b, __spv::MatrixLayout::PackedB)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when we should make the change: unused --> dynamic
packed_a and packed_b --> packed

@MrSidims MrSidims marked this pull request as draft November 11, 2022 14:17
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
@MrSidims MrSidims marked this pull request as ready for review November 14, 2022 14:26
@MrSidims MrSidims requested a review from dkhaldi November 14, 2022 14:26
@MrSidims

Copy link
Copy Markdown
Contributor Author

Manually tests on PVC, the patch works

@MrSidims

Copy link
Copy Markdown
Contributor Author

@dkhaldi please take a look

@dkhaldi dkhaldi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@steffenlarsen steffenlarsen merged commit 5a188c7 into intel:sycl Nov 17, 2022
yubingex007-a11y added a commit to yubingex007-a11y/llvm that referenced this pull request Dec 26, 2022
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