New extents implementation #212

crtrott · 2022-11-22T00:11:52Z

This replaces the extents implementation with something significanlty simpler. Need to do some more performance testing, but this is ready for review.

crtrott · 2023-02-01T23:50:10Z

You may just want to look at the new extents file not in the diff. I did just replace the file after all.

include/experimental/__p0009_bits/extents.hpp

crtrott · 2023-02-04T00:03:17Z

Some perf tests:

template <class Extents, class ... Args>
void extents_construction(benchmark::State& state, Extents, int R, Args ... args) {
  for (auto _ : state) {
    double val =0;
    for(int r=0; r<R; r++) {
       Extents ext(args...);
       val += ext.extent(0);
    }
    if(val<1.0*R*10) std::terminate();
  }
}

BENCHMARK_CAPTURE(
  extents_construction, int_int_int, stdex::dextents<int,3>(), 100000000, 100, 100, 100
);
BENCHMARK_CAPTURE(
  extents_construction, array_int_3, stdex::dextents<int,3>(), 100000000, std::array<int,3>{100, 100, 100}
);

In -O0 the new extents are faster. In O1 and above they are the same.

O0:

# stable
extents_construction/int_int_int 3966498599 ns   3957175517 ns            1
extents_construction/array_int_3 4942861061 ns   4931248140 ns            1
# new-extents-impl
extents_construction/int_int_int 1841957323 ns   1837625666 ns            1
extents_construction/array_int_3 3745596231 ns   3736797353 ns            1

O3:

#stable
extents_construction/int_int_int  108691256 ns    108436294 ns            6
extents_construction/array_int_3  108762740 ns    108507276 ns            6
#new-extents-impl
extents_construction/int_int_int  108782075 ns    108526909 ns            6
extents_construction/array_int_3  108849447 ns    108594120 ns            6

crtrott · 2023-02-04T00:22:38Z

Compile time test construction:

#include<experimental/mdspan>

constexpr size_t dyn = std::experimental::dynamic_extent;
template<class IndexT, size_t R, size_t ... Args>
struct ExtentsConstructor {
  using extents_t = std::experimental::extents<IndexT, R, Args...>;
  template<class ... DynArgs>
  static constexpr size_t value (DynArgs ... args) {
    extents_t ext(R,args...);
    return ext.extent(0) + ExtentsConstructor<IndexT, R-1, Args...>::value(args...);
  }
};

template<class IndexT, size_t ... Args>
struct ExtentsConstructor<IndexT, 0, Args...> {
  template<class ... DynArgs>
  static constexpr size_t value (DynArgs ... args) { return 0; }
};

size_t foo(int val1) {
  size_t value = 0;
  value += ExtentsConstructor<int32_t, 120, 3, 4, 5>::value(val1,4,5);
  value += ExtentsConstructor<uint32_t, 120, 3, 4, 5>::value(val1,4,5);
  value += ExtentsConstructor<int64_t, 120, 3, 4, 5>::value(val1,4,5);
  value += ExtentsConstructor<uint64_t, 120, 3, 4, 5>::value(val1,4,5);
  value += ExtentsConstructor<int32_t, 120, dyn,dyn,dyn>::value(val1,val1,val1);
  value += ExtentsConstructor<uint32_t, 120, dyn,dyn,dyn>::value(val1,val1,val1);
  value += ExtentsConstructor<int64_t, 120, dyn,dyn,dyn>::value(val1,val1,val1);
  value += ExtentsConstructor<uint64_t, 120, dyn,dyn,dyn>::value(val1,val1,val1);
  return value;
}

Simply compile as object file:
time g++ -O3 -c test.cpp -I${HOME}/Kokkos/mdspan/include

GCC 11.1 Old: 5.4s New 4.8s
Clang 15 Old: 4.0s New 3.9s

So looks like compile time for construction went down.

crtrott · 2023-02-04T00:34:08Z

Compile time access benchmark:

#include<experimental/mdspan>

constexpr size_t dyn = std::experimental::dynamic_extent;
template<class Extents, size_t R>
struct ExtentsConstructor {
  static constexpr size_t value (const Extents& ext) {
    return ext.extent(static_cast<int32_t>(R%Extents::rank())) +
           ext.extent(static_cast<int64_t>(R%Extents::rank())) +
           ext.extent(static_cast<uint32_t>(R%Extents::rank())) +
           ext.extent(static_cast<uint64_t>(R%Extents::rank())) +
            + ExtentsConstructor<Extents, R-1>::value(ext);
  }
};

template<class Extents>
struct ExtentsConstructor<Extents, 0> {
  static constexpr size_t value (const Extents&) { return 0; }
};

size_t foo(int val1) {
  size_t value = 0;
  {
    using ext_t = std::experimental::extents<int32_t, 3, 4, 5>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<int64_t, 3, 4, 5>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<uint32_t, 3, 4, 5>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<uint64_t, 3, 4, 5>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<int32_t, dyn, dyn, dyn>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<int64_t, dyn, dyn, dyn>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<uint32_t, dyn, dyn, dyn>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  {
    using ext_t = std::experimental::extents<uint64_t, dyn, dyn, dyn>;
    value += ExtentsConstructor<ext_t, 120>::value(ext_t(val1,4,5));
  }
  return value;
}

GCC 11.1 Old: 0.95 New 0.89
Clang 15 Old: 0.72 New 0.66

crtrott · 2023-02-04T00:35:19Z

I also ran the existing benchmarks and didn't see anything bad. So in general I think this is good to go.

mhoemmen

This is a lovely improvement -- it's much easier to read! Thank you for doing this!

My only concern is minor, that functions like get sometimes take int and sometimes take size_t. This could lead to conversion warnings.

include/experimental/__p0009_bits/extents.hpp

Co-Authored-By: Wesley Maxey <71408887+wmaxey@users.noreply.github.com>

dalg24

I don't love it but it is mostly fine

dalg24 · 2023-04-03T18:20:49Z

include/experimental/__p0009_bits/layout_stride.hpp

@@ -272,7 +239,11 @@ struct layout_stride {
 #else
      : __base_t(__base_t{__member_pair_t(
 #endif
+#ifndef _MDSPAN_NEW_EXTENT2S


What is this macro?

leftover from when you could switch in the PR between the old and new implementation, gonna remove.

dalg24 · 2023-04-03T22:03:00Z

include/experimental/__p0009_bits/extents.hpp

+    else
+      return 0;
+  }
+  template <size_t r> MDSPAN_INLINE_FUNCTION constexpr static size_t get() {


This overload is never used.

I still kinda would go for consistency here across the different things which looks similar?

CMakeLists.txt

include/experimental/__p0009_bits/extents.hpp

mhoemmen · 2023-04-03T22:34:23Z

include/experimental/__p0009_bits/extents.hpp

+  constexpr static T get(size_t) { return T(); }
+  template <size_t> MDSPAN_INLINE_FUNCTION constexpr static T get() {
+    return T();


Calling get on an empty array is definitely a precondition violation. Thus, would you consider using __builtin_unreachable() with GCC, or std::unreachable with C++23, with functions like this? This works fine in practice with constexpr functions.

I really don't want to add ifdefs and builtins inside this implementation only code.

I think we should at least assert since it will be called on out of bounds access and static_assert where possible

mhoemmen · 2023-04-03T22:36:41Z

include/experimental/__p0009_bits/extents.hpp

-    : __storage_{
-#else
-    : __base_t(__base_t{typename __base_t::__stored_type{
+  constexpr maybe_static_array(const std::span<T, N> &vals) {


Should we consider passing span by value instead of by const reference?

nmm0

I think my biggest issue is returning values if the size of maybe_static_array is 0. I would rather specialize and maybe even assert if the get functions are accessed

CMakeLists.txt

include/experimental/__p0009_bits/extents.hpp

nmm0 · 2023-04-03T22:05:42Z

include/experimental/__p0009_bits/extents.hpp

+  constexpr static size_t m_size = sizeof...(Values);
+  constexpr static size_t m_size_dynamic =
+      _MDSPAN_FOLD_PLUS_RIGHT((Values == dyn_tag), 0);


These can be moved into the respective functions

actually m_size_dynamic is used right below here, I guess if I moved that member to the end so the constexpr accessor functions are already defined? But I don't see a big benefit since they are anyway private.

It's just less code. I'm not dead set on this

nmm0 · 2023-04-03T22:10:17Z

include/experimental/__p0009_bits/extents.hpp

+  MDSPAN_TEMPLATE_REQUIRES(class T, size_t N,
+                           /* requires */ (N == m_size_dynamic && N == 0))
  MDSPAN_INLINE_FUNCTION
-  explicit constexpr extents(Integral... exts) noexcept
-#if defined(_MDSPAN_USE_ATTRIBUTE_NO_UNIQUE_ADDRESS)
-    : __storage_{
-#else
-    : __base_t(__base_t{typename __base_t::__stored_type{
+  constexpr maybe_static_array(const std::array<T, N> &) : m_dyn_vals{} {}


Suggested change

MDSPAN_TEMPLATE_REQUIRES(class T, size_t N,

/* requires */ (N == m_size_dynamic && N == 0))

MDSPAN_INLINE_FUNCTION

explicit constexpr extents(Integral... exts) noexcept

#if defined(_MDSPAN_USE_ATTRIBUTE_NO_UNIQUE_ADDRESS)

: __storage_{

#else

: __base_t(__base_t{typename __base_t::__stored_type{

constexpr maybe_static_array(const std::array<T, N> &) : m_dyn_vals{} {}

MDSPAN_TEMPLATE_REQUIRES(class T,

/* requires */ (m_size_dynamic == 0))

MDSPAN_INLINE_FUNCTION

constexpr maybe_static_array(const std::array<T, 0> &) : m_dyn_vals{} {}

Doesn't work because of std::array not working on device.

Oh weird, my diff got messed up idk why it grabbed some of the old code. The only change here was to simplify

MDSPAN_TEMPLATE_REQUIRES(class T, size_t N, /* requires */ (N == m_size_dynamic && N == 0)) MDSPAN_INLINE_FUNCTION constexpr maybe_static_array(const std::array<T, N> &) : m_dyn_vals{} {}

to

MDSPAN_TEMPLATE_REQUIRES(class T, /* requires */ (m_size_dynamic == 0)) MDSPAN_INLINE_FUNCTION constexpr maybe_static_array(const std::array<T, 0> &) : m_dyn_vals{} {}

-- you are already using array here so this isn't introducing it in a new place

include/experimental/__p0009_bits/extents.hpp

mhoemmen · 2023-04-03T22:38:07Z

include/experimental/__p0009_bits/extents.hpp

-  template<class OtherIndexType, size_t... RHS>
+template <class IndexType, size_t... Extents> class extents {
+public:
+  // typedefs for integral types used


Should we consider checking the Mandate that IndexType is a signed or unsigned integral type? That would effectively make the code self-documenting, so that the comment would become superfluous.

Yeah I can do that.

include/experimental/__p0009_bits/extents.hpp

mhoemmen

Hi Christian! This is much more concise; I like it! I just made a few optional suggestions.

Co-authored-by: Mark Hoemmen <mhoemmen@users.noreply.github.com> Co-authored-by: Nicolas Morales <nmmoral@sandia.gov>

include/experimental/__p0009_bits/extents.hpp

nmm0

I think overall I would want the out of bounds checks to be asserted/static_asserted when able but otherwise looks good!

include/experimental/__p0009_bits/extents.hpp

nmm0 · 2023-04-04T16:12:52Z

include/experimental/__p0009_bits/extents.hpp

+  constexpr static T get(size_t) { return T(); }
+  template <size_t> MDSPAN_INLINE_FUNCTION constexpr static T get() {
+    return T();


I think we should at least assert since it will be called on out of bounds access and static_assert where possible

include/experimental/__p0009_bits/extents.hpp

nmm0 · 2023-04-04T16:14:42Z

include/experimental/__p0009_bits/extents.hpp

+  constexpr static size_t m_size = sizeof...(Values);
+  constexpr static size_t m_size_dynamic =
+      _MDSPAN_FOLD_PLUS_RIGHT((Values == dyn_tag), 0);


It's just less code. I'm not dead set on this

nmm0 · 2023-04-04T16:19:20Z

include/experimental/__p0009_bits/extents.hpp

+  MDSPAN_TEMPLATE_REQUIRES(class T, size_t N,
+                           /* requires */ (N == m_size_dynamic && N == 0))
  MDSPAN_INLINE_FUNCTION
-  explicit constexpr extents(Integral... exts) noexcept
-#if defined(_MDSPAN_USE_ATTRIBUTE_NO_UNIQUE_ADDRESS)
-    : __storage_{
-#else
-    : __base_t(__base_t{typename __base_t::__stored_type{
+  constexpr maybe_static_array(const std::array<T, N> &) : m_dyn_vals{} {}


Oh weird, my diff got messed up idk why it grabbed some of the old code. The only change here was to simplify

MDSPAN_TEMPLATE_REQUIRES(class T, size_t N, /* requires */ (N == m_size_dynamic && N == 0)) MDSPAN_INLINE_FUNCTION constexpr maybe_static_array(const std::array<T, N> &) : m_dyn_vals{} {}

to

MDSPAN_TEMPLATE_REQUIRES(class T, /* requires */ (m_size_dynamic == 0)) MDSPAN_INLINE_FUNCTION constexpr maybe_static_array(const std::array<T, 0> &) : m_dyn_vals{} {}

-- you are already using array here so this isn't introducing it in a new place

nmm0 · 2023-04-04T16:31:30Z

include/experimental/__p0009_bits/extents.hpp

+  public static_array_impl<0, T, Values...>  {

-template <class ThisIndexType, size_t... Extents>
-class extents
-#if !defined(_MDSPAN_USE_ATTRIBUTE_NO_UNIQUE_ADDRESS)
-  : private detail::__no_unique_address_emulation<
-      detail::__partially_static_sizes_tagged<detail::__extents_tag, ThisIndexType , size_t, Extents...>>
-#endif
-{
 public:


Very minor point, but both publics here are redundant

The above thing is not the same (where you say m_size_dynamic==0). Think N =1 and m_size_dynamic = 0, your case would take it, whats there right now doesn.t

mhoemmen · 2023-04-04T17:18:50Z

@nmm0 wrote:

I think we should at least assert since it will be called on out of bounds access and static_assert where possible

regarding

 template <size_t> MDSPAN_INLINE_FUNCTION constexpr static T get() {
    return T();
}

another approach would be to use the C++23 feature std::unreachable(), or one of the compiler-specific equivalents, like GCC's __builtin_unreachable().

Co-authored-by: Mark Hoemmen <mhoemmen@users.noreply.github.com>

…product_kokkos_impl [ready] kokkos impl `triangular_matrix_{left,right}_product`

crtrott mentioned this pull request Nov 22, 2022

extents::operator[] #103

Closed

crtrott force-pushed the new-extents-impl branch 3 times, most recently from f206c73 to 3fa2214 Compare February 1, 2023 22:52

crtrott marked this pull request as ready for review February 1, 2023 23:00

dalg24 reviewed Feb 3, 2023

View reviewed changes

mhoemmen reviewed Feb 6, 2023

View reviewed changes

crtrott added 18 commits March 15, 2023 13:19

Add new implementation of extents and maybe_static_array

08a38fd

Add changes in layouts and mdspan to support new extents

b14a750

Enable new extents implementation

c17c896

Disable temporarly submdspan tests

f99b51c

Fix a bunch of unsigned/signed warnings in new extents impl

f0ff77c

Replace requires clause with C++14 backward macro

33ff7da

Protect use of span in new extents impl

001a161

Make new extents impl compile with C++14

3aa022b

Add function markup to new extents implementation

cd54023

Get new extents to work with nvcc

15e6224

Fix a bit more stuff for NVCC build

6e93f6f

Clang-format

e873312

Delete old extents

65d550e

Renamed new extents file

b83ac2f

Remove implementation detail files for old extents

e550a7f

Get rid of some old extents use code

02481f7

Add missing copyright

69a9abb

Update comments in extents implementation

ea1d9ca

crtrott and others added 2 commits March 15, 2023 13:30

Fix MSVC 14 issue

49e1162

Co-Authored-By: Wesley Maxey <71408887+wmaxey@users.noreply.github.com>

Add static assertion to check saneness of types in maybe_static_array

cd39b4c

crtrott force-pushed the new-extents-impl branch from f3ad21e to cd39b4c Compare March 16, 2023 21:48

Fix use of int and don't compile benchmarks in C++23 mode

002f2ad

crtrott mentioned this pull request Mar 28, 2023

Build failing on GCC arm due to _N defined as a macro #187

Closed

dalg24 reviewed Apr 3, 2023

View reviewed changes

dalg24 approved these changes Apr 3, 2023

View reviewed changes

mhoemmen reviewed Apr 3, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mhoemmen reviewed Apr 3, 2023

View reviewed changes

include/experimental/__p0009_bits/extents.hpp Outdated Show resolved Hide resolved

mhoemmen reviewed Apr 3, 2023

View reviewed changes

nmm0 requested changes Apr 3, 2023

View reviewed changes

mhoemmen reviewed Apr 3, 2023

View reviewed changes

include/experimental/__p0009_bits/extents.hpp Show resolved Hide resolved

mhoemmen approved these changes Apr 3, 2023

View reviewed changes

crtrott and others added 6 commits April 3, 2023 17:40

Apply suggestions from code review

2a782c2

Co-authored-by: Mark Hoemmen <mhoemmen@users.noreply.github.com> Co-authored-by: Nicolas Morales <nmmoral@sandia.gov>

Make maybe-static-array dyn_map_t private

588a50a

Make static_array inherit from its impl type, and check extents mandate

b568d51

Remove some leftover newextents/oldextents stuff

d9f4715

Fix two build issues on NVHPC

952be00

Remove unused function in the scan

e78a29c

mhoemmen reviewed Apr 4, 2023

View reviewed changes

include/experimental/__p0009_bits/extents.hpp Outdated Show resolved Hide resolved

nmm0 approved these changes Apr 4, 2023

View reviewed changes

Fix build of examples with bracket operator

c93cd34

crtrott force-pushed the new-extents-impl branch from 9396793 to c93cd34 Compare April 4, 2023 17:16

Apply suggestions from code review

2d8f8ef

Co-authored-by: Mark Hoemmen <mhoemmen@users.noreply.github.com>

crtrott merged commit a9489ae into kokkos:stable Apr 4, 2023

crtrott deleted the new-extents-impl branch April 4, 2023 17:25

mhoemmen added a commit to mhoemmen/mdspan that referenced this pull request Jul 26, 2023

Merge pull request kokkos#212 from NexGenAnalytics/triangular-matrix-…

0804c2e

…product_kokkos_impl [ready] kokkos impl `triangular_matrix_{left,right}_product`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New extents implementation #212

New extents implementation #212

crtrott commented Nov 22, 2022 •

edited

Loading

crtrott commented Feb 1, 2023

crtrott commented Feb 4, 2023 •

edited

Loading

crtrott commented Feb 4, 2023

crtrott commented Feb 4, 2023

crtrott commented Feb 4, 2023

mhoemmen left a comment

dalg24 left a comment

dalg24 Apr 3, 2023

crtrott Apr 3, 2023

dalg24 Apr 3, 2023

crtrott Apr 3, 2023

mhoemmen Apr 3, 2023

crtrott Apr 3, 2023

nmm0 Apr 4, 2023

mhoemmen Apr 3, 2023

crtrott Apr 3, 2023

nmm0 left a comment

nmm0 Apr 3, 2023

crtrott Apr 3, 2023

crtrott Apr 3, 2023

nmm0 Apr 4, 2023

nmm0 Apr 3, 2023

crtrott Apr 3, 2023

nmm0 Apr 4, 2023

mhoemmen Apr 3, 2023 •

edited

Loading

crtrott Apr 3, 2023

mhoemmen left a comment

nmm0 left a comment

nmm0 Apr 4, 2023

nmm0 Apr 4, 2023

nmm0 Apr 4, 2023

nmm0 Apr 4, 2023

crtrott Apr 4, 2023

mhoemmen commented Apr 4, 2023

New extents implementation #212

New extents implementation #212

Conversation

crtrott commented Nov 22, 2022 • edited Loading

crtrott commented Feb 1, 2023

crtrott commented Feb 4, 2023 • edited Loading

crtrott commented Feb 4, 2023

crtrott commented Feb 4, 2023

crtrott commented Feb 4, 2023

mhoemmen left a comment

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen left a comment

Choose a reason for hiding this comment

nmm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhoemmen commented Apr 4, 2023

crtrott commented Nov 22, 2022 •

edited

Loading

crtrott commented Feb 4, 2023 •

edited

Loading

mhoemmen Apr 3, 2023 •

edited

Loading