intel
update the matrix spec based on new use argument
By the way, I corrected a lot of typos that are still relevant for the new document here: https://github.com/intel/llvm/pull/6525/files
Basically if you ignore the new sections I added there (#### Binary Multiply and Add and ##### wi_data as an marray for Nvidia® Tensor Cores) everything else is just small corrections that you could consider adding here.
By the way, I corrected a lot of typos that are still relevant for the new document here: https://github.com/intel/llvm/pull/6525/files
Basically if you ignore the new sections I added there (#### Binary Multiply and Add and #####
wi_dataas anmarrayfor Nvidia® Tensor Cores) everything else is just small corrections that you could consider adding here.
Yes, I will add these corrections here. Thanks.
I have a question about the old specification, which is now named "sycl_ext_oneapi_deprecated_matrix_no_use". Is that version of the API still supported? Does (or will) the implementation print a warning message for applications that use the old API?
Or, is the old API no longer supported, and it is (or will be) removed?
Both options are OK for an experimental extension like this. However, we should treat the documentation differently in the two cases.
I have a question about the old specification, which is now named "sycl_ext_oneapi_deprecated_matrix_no_use". Is that version of the API still supported?
@gmlueck, yes, this version is still supported and will be supported until the new API is implemented on the GPU. Currently, it is only implemented on the CPU.
Does (or will) the implementation print a warning message for applications that use the old API?
No it does not because the new API is not supported on the GPU yet. So for users, they still need to use the old API until we remove it for oneAPI 2023.1 release.
Or, is the old API no longer supported, and it is (or will be) removed? The old API will be removed for oneAPI 2023.1 release.
Both options are OK for an experimental extension like this. However, we should treat the documentation differently in the two cases.
For this release, we want to keep the old API unchanged and supported. At the same time, we move towards the new API. Once the new API is also supported on the GPU, we can completely remove the old API.
For this release, we want to keep the old API unchanged and supported. At the same time, we move towards the new API. Once the new API is also supported on the GPU, we can completely remove the old API.
Sorry for the delayed response. It looks like both extensions define the same types in different ways. For example, the new version has:
namespace sycl::ext::oneapi::experimental::matrix {
template <typename T, use Use,
size_t Rows=sycl::dynamic_extent, size_t Cols=sycl::dynamic_extent,
layout Layout = layout::unused, typename Group = sub_group>
struct joint_matrix {
joint_matrix(Group g) {}
};
}
and the old version has:
namespace sycl::ext::oneapi::experimental::matrix {
template <typename T, size_t Rows=sycl::dynamic_extent, size_t Cols=sycl::dynamic_extent,
matrix_layout Layout = matrix_layout::row_major, typename Group = sub_group>
struct joint_matrix {
joint_matrix(Group g) {}
};
}
How can you support both simultaneously? How does the user determine whether they get the new vs. the old API?
How can you support both simultaneously? How does the user determine whether they get the new vs. the old API?
they can do this by specifying the SYCL_EXT_ONEAPI_MATRIX feature test macro. See what we have today here: https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/matrix/matrix.hpp
they can do this by specifying the SYCL_EXT_ONEAPI_MATRIX feature test macro. See what we have today here: https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/matrix/matrix.hpp
This is not how the feature-test macros are supposed to work. The implementation determines the value of the feature-test macro, not the application. (The extension specification says this in the section "Feature test macro".) If you want the user to choose the API via a macro, we should choose a different macro name. For example, SYCL_EXT_ONEAPI_MATRIX_LEGACY_API could enable the old API.
You have this inconsistency throughout the spec. In many places, you refer to the three matrices as a, b, and c. However, the use enum refers to them as a, b, and accumulator. You should decide on one or the other and then use it consistently throughout. Since you call the first two matrices a and b, it seems natural to me to call the third matrix c. Is there a good reason to call the third matrix accumulator?
@JackAKirk what do you think?
Whenever you rename (or move) a specification, you should search the repo and see if there are any links to the old name. In your case, there are several references from "sycl/ReleaseNotes.md". These should be updated:
- Point to the old file, or
- Point to the new file, or
- Change the wording to remove the link.
Thank you! yes, I will point to the new file
You have this inconsistency throughout the spec. In many places, you refer to the three matrices as a, b, and c. However, the use enum refers to them as a, b, and accumulator. You should decide on one or the other and then use it consistently throughout. Since you call the first two matrices a and b, it seems natural to me to call the third matrix c. Is there a good reason to call the third matrix accumulator?
@JackAKirk what do you think?
I think that the argument for having a,b and accumulator is that the programming idiom is either
d=a*b+c
Where the d matrix would also have to be use::c(use::accumulator)
or as in most use cases:
c=a*b+c
use::accumulator is a more general name than use::c. However I think either choice is fine.
@intel/dpcpp-doc-reviewers, any more reviews on this? It will be good if we can merge this ASAP.
@intel/dpcpp-doc-reviewers, can you please help merge this if there are no more comments?
@dkhaldi, please move deprecated extension to https://github.com/intel/llvm/tree/sycl/sycl/doc/extensions/deprecated
@dkhaldi, please move deprecated extension to https://github.com/intel/llvm/tree/sycl/sycl/doc/extensions/deprecated
done
https://github.com/orgs/intel/teams/dpcpp-doc-reviewers, please help merge.
Sorry for the delay. This is the next thing on my review list.
Since you moved the deprecated version of the API spec to "doc/extensions/deprecated", there isn't any need for the "doc/extensions/experimental/sycl_ext_oneapi_matrix" directory. Can you move the spec from:
sycl/doc/extensions/experimental/sycl_ext_oneapi_matrix/sycl_ext_oneapi_matrix.asciidoc
to:
sycl/doc/extensions/experimental/sycl_ext_oneapi_matrix.asciidoc
The folder is still necessary to add CUDA backend additional specific API (like marray-based element wise operations). see #6968.