-
Notifications
You must be signed in to change notification settings - Fork 217
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
Documentation for oneAPI data management backend primitives #3008
base: main
Are you sure you want to change the base?
Conversation
Would be helpful if we could merge this PR first: So that we could then check that this one isn't introducing new warnings that might be masked by previous ones in the CI logs. |
@@ -285,11 +376,22 @@ class ndview : public ndarray_base<axis_count, order> { | |||
} | |||
|
|||
#ifdef ONEDAL_DATA_PARALLEL | |||
/// Returns a copy of the element of 1d immutable view located in USM at specified location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it execute operations in a queue if the data depends on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I do not fully understand the question. This operation is about the same as other sycl operations, it will be included into the flow of dependencies along with the other operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose that the array is the result of a computation like
The operation that calculates the numbers in the array returns a queue, and might not have been completed by the time that .at
is called.
Will the call to .at
force calculations of the operations in the queues before returning a scalar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, in case the events triggered by deps
vector correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to mention that in the docs.
@@ -539,14 +712,26 @@ inline sycl::event fill(sycl::queue& q, | |||
} | |||
#endif | |||
|
|||
/// Multidimensional array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it differ from sycl's own class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think sycl has own multidimensional data representations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it does have a 1D marray
. Lots of the usage for views is as 1D containers, so I was curious about their usage over sycl's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think besides of multidimensionality, our data structures are interoperable in the sense one can easily view the same data block as dal::table, dal::array, dal::backend::primitives::ndview, etc.
Also here we implement convenience functions like slice, transpose, to_host, etc.
With sycl own structures the amount of code inside oneDAL algorithmic kernels would be much bigger. Anyway we would need to wrap some common operations into functions. This class groups those common operations on the data together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to add a hint to prefer these custom classes instead of sycl's.
@rakshithgb-fujitsu, @keeranroth can you please provide the feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments look good for the code that is present, so I've refrained from commenting on the code. But for my own information, is there a plan to make the views constexpr, or to have the layout separate from the data, so that the offsets into arrays / pointers / multi-dimensional arrays can be calculated at compile time? And does the implementation support arbitrary shapes and strides, e.g. having a layout of the shape (a, (b, c), d), which is a 3 dimensional object, where the second dimension is a 2-dimensional object. This is useful in terms of doing things like cache-blocking. Not for this code review, but it would be great to see functionality like this, if it doesn't already exist
No, there are no such plans for now. But I understand that it might be beneficial to have ndarrays of constexpr sizes to represent some data blocks used in intermediate computations.
I think it would be more correct to answer "no" to this question. Because:
|
ndview
,ndarray
and related primitives are documented.PR completeness and readability