mdspan icon indicating copy to clipboard operation
mdspan copied to clipboard

Change MDSPAN_INLINE_FUNCTION to _MDSPAN_INLINE_FUNCTION (was "Rename _MDSPAN_HOST_DEVICE to MDSPAN_HOST_DEVICE")

Open mhoemmen opened this issue 3 years ago • 0 comments

@youyu3 @crtrott

New issue

I revised this issue on 2022/09/06, based on offline discussion with @crtrott . We do not want to give users the impression that they are allowed to use the MDSPAN_INLINE_FUNCTION macro. It is for mdspan implementers only. It may be used in mdspan tests and examples, but it may NOT be used outside mdspan.

Thus, we need to add an underscore prefix to the MDSPAN_INLINE_FUNCTION macro, to make it clear to users that it is an implementation detail and not for use outside mdspan. This will make it consistent with the _MDSPAN_HOST_DEVICE macro, which must retain its underscore prefix for the same reason.

The former text of this issue is below, and no longer applies.

Former issue

Based on my comment here: https://github.com/kokkos/mdspan/pull/184#issuecomment-1235715279

Rename _MDSPAN_HOST_DEVICE to MDSPAN_HOST_DEVICE, or add a new MDSPAN_HOST_DEVICE macro that has the same definition as _MDSPAN_HOST_DEVICE. This would promote the macro to be just as usable as MDSPAN_INLINE_FUNCTION. (An underscore prefix suggests that it is an implementation detail.) Here are some reasons.

  1. It's not correct to apply the inline keyword to lambdas.
  2. In fact, existing tests already use _MDSPAN_HOST_DEVICE for the lambdas given to dispatch. This suggests to users that _MDSPAN_HOST_DEVICE is not really an implementation detail.
  3. Some mdspan implementation strategies (e.g., []<size_t ...>{ /* stuff */ }) depend on lambdas, so we need a "blessed" way to declare lambdas __host__ __device__.
  4. The inline keyword changes ODR semantics. Forcing use of inline with __host__ __device__, while not so important for a header-only library, could be bad for users' code that relies on RDC or other vendors' equivalent constructs.

mhoemmen avatar Sep 02 '22 17:09 mhoemmen