[REVIEW] Span implementation. #399
Conversation
|
Any reason to adopt the XGBoost based one instead of the cudf one (https://github.com/rapidsai/cudf/blob/branch-22.02/cpp/include/cudf/utilities/span.hpp)? AFAIK, this cudf based implementation will eventually go to libcu++. |
|
Hi, we haven't decided which implementation to use. The one from xgboost is closer to |
I see. I guess the benefit of the cudf one is that it has host_span & device_span so allows us to specify whether the pointer is for host memory or device memory. I guess this is beneficial unless CUDA's unified memory model advances to the level that this distinction is unnecessary. |
|
Yup, having safeguards is certainly useful. But the one in cudf doesn't support static extend https://github.com/rapidsai/cudf/blob/7ac8aac8d9174ecfcb06f56199c641bf3d869961/cpp/include/cudf/utilities/span.hpp#L42 . If we were to adopt it static extend must be integrated (not sure what's the best way yet). Please note that static extend can be used in both host and device. |
|
Added a template parameter to distinguish host and device span. |
|
I should rename |
|
|
The implementation is largely ported from xgboost with some simplification and cleanups. The one in XGBoost was modeled after core guideline support library instead of std, which was still a draft back then. The one in this PR uses plain pointer as iterator and don't have bound check in release model. Tests. Distinguish host/device. More tests. Rename. Rename + flexiable indexing. lint
|
This PR has been labeled |
|
Any progress on this one? TreeExplainer in cuML (rapidsai/cuml#4473) can benefit from a Span implementation. (Right now, it has its own small Span class.) Or is this going to be part of the upcoming MDSpan (#437) ? |
|
One can use mdspan to define span equivalence, but the underlying procedures are lot more complicated. |
|
Got it. In that case, TreeExplainer could potentially use mdspan instead. |
cpp/include/raft/common/span.hpp
Outdated
| * - Otherwise, dynamic_extent. | ||
| */ | ||
| template <std::size_t Extent, std::size_t Offset, std::size_t Count> | ||
| struct extent_value_t |
There was a problem hiding this comment.
Just a side thought- it would be super useful to be able to implicitly convert between a _mdspan_vector_t and a _span_t. For example, this would allow the RAFT API to accept an _mdspan_vector_t and pass it a _span_t
There was a problem hiding this comment.
@cjnolet We have some integration work to do. For instance, the dynamic_extent in this PR is duplicated with the one in stdex.
There was a problem hiding this comment.
We can polish after having the base implementation and make a list of additional features we want, like conversion between span and 1-dim mdspan, padding for arrays, resize for arrays.
There was a problem hiding this comment.
@trivialfis, I've looked over your changes and it's looking like this is about ready to merge. What do you think? Can you capture the follow-on items in Github issues so we don't lose them?
There was a problem hiding this comment.
Got it, I will remove all the postfix for span and mdspan.
My apologies, it was ported from xgboost, which supports enumerating devices. Will remove those calls. |
* Update license. * Add doc strings. * Move to top level namespace. * Remove set device.
|
Changed base to 22.04. |
|
Hey, is there anything else I need to do for this PR? Maybe retrigger the CI? |
|
@gpucibot merge |
| } | ||
|
|
||
| private: | ||
| size_type size_{0}; |
There was a problem hiding this comment.
This implementation is missing a principle characteristic of span where a span with a static extent has only a single pointer data member. This is usually accomplished by doing something like:
template <typename T, std::size_t Extent>
struct span_storage{
span_storage(T* p, std::size_t /*unused*/) : data_{p} {}
constexpr std::size_t size(){ return Extent; };
T* data_ = nullptr;
// no size member
};
template <typename T>
struct span_storage<T, dynamic_extent>{
span_storage(T* p, std::size_t s) : data_{p}, size_{s} {}
constexpr std::size_t size() { return size_; };
private:
T* data_ = nullptr;
std::size_t size_;
};
...
template <typename T, std::size_t Extent = dynamic_extent>
struct span{
...
private:
span_storage<T, Extent> storage_;
}
This further guarantees the compiler will optimize according to the static extent.
There was a problem hiding this comment.
Thank you for the suggestion, will follow up on this #511 .
The implementation is largely ported from xgboost with some simplification and cleanups.
The one in XGBoost was modeled after the core guideline support library instead of std, which
was still a draft back then. The one in this PR uses a plain pointer as the iterator and doesn't
have bound check in release mode. ISO span uses
size_typefor index operator, this PR definesa template parameter to allow indexing using other integer types.