[FEA] Implement matrix transpose with mdspan. #739
Conversation
trivialfis
commented
Jul 12, 2022
•
trivialfis
commented
- Implement a transpose function that works on both column and row major matrix.
- sub-matrix is supported as well.
* Implement a transpose function that works on both column and row major matrix.
trivialfis
added
feature request
That's correct @divyegala. In preparation for our being able to support dispatch to multiple execution environments (e.g. CPU/GPU) in the future, we are coding our public APIs directly to |
|
@trivialfis with Corey's confirmation on this, can we use the vanilla mdspan API? Also we should leave out the overload that returns our implementation of |
|
Thank you for the review, I hid the functions into tests and changed the input parameters to vanilla mdspan. |
| @@ -79,6 +80,55 @@ void transpose(math_t* inout, int n, cudaStream_t stream) | |||
| }); | |||
| } | |||
|
|
|||
| template <typename T, typename LayoutPolicy> | |||
| void transpose_row_major_impl(handle_t const& handle, | |||
| device_matrix_view<T, LayoutPolicy> in, | |||
There was a problem hiding this comment.
These should also be vanilla mdspan? I am surprised we are able to construct a device_matrix_view with mdspan
There was a problem hiding this comment.
I'm not sure if this should be normal span. It's a safe guard that prevents users from passing the wrong pointer.
There was a problem hiding this comment.
It won't be a breaking change when we support host mdspan since we will be just relaxing the constraint.
There was a problem hiding this comment.
I think we might be okay on the impl side. Because this isn't directly exposed to users, we have the freedom to change this as needed.
There was a problem hiding this comment.
device_matrix_view doesn't do any check on the pointer's underlying memory though, right? We can construct it even with a host pointer/mdspan
There was a problem hiding this comment.
I think that's why we should just accept vanilla mdspan all the way through
There was a problem hiding this comment.
It's a compile time guard. Given a host mdspan it won't compile.
There was a problem hiding this comment.
@trivialfis can we accept vanilla mdspan on our public API, and either use pointers or vanilla mdspan all the way after that? In the future, we will discuss the design of an "execution environment" that will allow us to handle memory guards better (compile time/run time). Until then, it will be easier to use the standard and then refactor if needed
There was a problem hiding this comment.
Got it, will make the changes. Could you please loop me into the discussion of "execution environment"? I think that might benefit the development for XGBoost.
There was a problem hiding this comment.
Changed to using vanilla mdspan.
|
rerun tests |
|
@gpucibot merge |
