kokkos / stdBLAS

Reference Implementation for stdBLAS
Other
118 stars 22 forks source link

Fix assumption that mdspan components live in std::experimental namespace #265

Closed mhoemmen closed 1 month ago

mhoemmen commented 7 months ago

PR #263 (merged today) required a hack to get use of submdspan to build correctly. The issue is that this repository currently assumes that #include <experimental/mdspan> puts all mdspan components (including submdspan) in namespace std::experimental. In fact, this repository hard-codes the namespaces of mdspan components as std::experimental.

This assumption is bad for two reasons. First, eventually compilers will get complete mdspan and submdspan implementations, and they will live in the std namespace. Second, user-space libraries are not allowed to put symbols in the std namespace. An application or library that wants to use the reference mdspan implementation specifically should either assume the default Kokkos, or should use the macros that the reference mdspan implementation already defines in order to control the default namespace.

<experimental/mdspan> just does the following.

#pragma once

#ifndef MDSPAN_IMPL_STANDARD_NAMESPACE
  #define MDSPAN_IMPL_STANDARD_NAMESPACE std
#endif

#ifndef MDSPAN_IMPL_PROPOSED_NAMESPACE
  #define MDSPAN_IMPL_PROPOSED_NAMESPACE experimental
#endif

#include "../mdspan/mdspan.hpp"

// backward compatibility import into experimental
namespace MDSPAN_IMPL_STANDARD_NAMESPACE {
  namespace MDSPAN_IMPL_PROPOSED_NAMESPACE {
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::mdspan;
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::extents;
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_left;
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_right;
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_stride;
    using ::MDSPAN_IMPL_STANDARD_NAMESPACE::default_accessor;
  }
}

The right thing for this repository to do would be to add a header for including mdspan (and submdspan), and import the relevant symbols via using declarations.

#pragma once

#ifndef MDSPAN_IMPL_STANDARD_NAMESPACE
#define MDSPAN_IMPL_STANDARD_NAMESPACE Kokkos
#endif

#ifndef LINALG_IMPL_NAMESPACE
#define LINALG_IMPL_NAMESPACE linalg

#include "../mdspan/mdspan.hpp"

namespace LINALG_IMPL_NAMESPACE {
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::extents;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::mdspan;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::submdspan;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_left;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_right;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::layout_stride;
  using ::MDSPAN_IMPL_STANDARD_NAMESPACE::default_accessor;
  // ... etc. ...
}
mhoemmen commented 1 month ago

I've since fixed this in the tests.