MB hacks, required changes and brainstorming for latest master #115

psiha · 2023-04-11T11:32:13Z

…led builds.

jfalcou

Some of those changes will probably disappear with fix-99-4 as the rework of internal storage for stride and shape ditch a lot of this code. I am gonna try to finalise and merge 99-94 then apply this.

include/kwk/detail/kumi.hpp

include/kwk/utility/container/stride.hpp

test/tts.hpp

…trivial by automatically.

…per fix would be to make this configurable :grimace:).

- was broken for 'partially dynamic' shapes? - did not 'exist' at all for fully static shapes - had duplicated const and nonconst code (with an extraneous assertion about mutating static values for the const version). empty_tuple is homogeneous? Minor stylistic fixes.

…per fix would be to make this configurable :grimace:).

…ith of_size() (which uses to_descriptor() - why?).

- changed base_type to uint16 to match the (new) joker default (awaiting a proper solution) - added conversion operators to Content/value - attempt to make axis_ w/ empty Content an empty class.

cmake/make_unit.cmake

jfalcou · 2023-08-01T13:55:24Z

include/kwk/detail/sequence/helpers.hpp

@@ -114,7 +114,7 @@ namespace kwk::__
  // Type short-cut for internal representation of types in axis
  //====================================================================================================================
  template<typename T>        struct stored_type : T {};
-  template<concepts::joker T> struct stored_type<T> { using type = std::int32_t; };
+  template<concepts::joker T> struct stored_type<T> { using type = std::uint16_t; };


Not a fan to have that hardcoded. If you want it globally selectable instead of putting short everywhere in shape definition, I can live with a KWK_DEFAULT_DIMENSION_TYPE thing.

right right, i have a lot of other changes incoming (still strugling with fully moving to new kiwaku, quite time consuming) so please wait a day or two until i push those - then will have to have another 'facetime' i'm pretty sure 🙈 😁

in advance: the main problem is that if we support heterogeneous shape containers, defaulting runtime-sized dimensions to a fixed integral type defined in joker is inconsistent/incomplete: one should be able to choose that also - for now i've put in an nttp based mechanism that most resembles what you do elswhere - so you'd say shape<_[short{}]> to get a 16bit untyped runtimesized axis

actually this whole joker idea really 'broke my back', lost a lot of time on that - it made my code more complicated (as well as the changes i required in kiwaku) - the previous approach where a dynamic/runtime-specified dimension/axis was specified simply with a zero/default-constructed value (using/counting on the fact/assumption that no one will ever need or use a fixed zero statically sized axis - what would be the point of that? i.e. such a shape will always have a zero count/volume)

in advance: the main problem is that if we support heterogeneous shape containers, defaulting runtime-sized dimensions to a fixed integral type defined in joker is inconsistent/incomplete: one should be able to choose that also - for now i've put in an nttp based mechanism that most resembles what you do elswhere - so you'd say shape<_[short{}]> to get a 16bit untyped runtimesized axis

That's already available (see https://jfalcou.github.io/kiwaku/glossary.html#glossary-extent)

https://godbolt.org/z/ov5r59jh5

actually this whole joker idea really 'broke my back', lost a lot of time on that - it made my code more complicated (as well as the changes i required in kiwaku) - the previous approach where a dynamic/runtime-specified dimension/axis was specified simply with a zero/default-constructed value (using/counting on the fact/assumption that no one will ever need or use a fixed zero statically sized axis - what would be the point of that? i.e. such a shape will always have a zero count/volume)

I needed something regular for the other algorithms and shape based operation to work with less complexity.

That's already available (see https://jfalcou.github.io/kiwaku/glossary.html#glossary-extent)

https://godbolt.org/z/ov5r59jh5

ok great in one sense but in the other it makes it even more complicated :/ for example now i have to detect the as<> thingy in addition to jokers as 'dynamic axes'...

i've already spent entire days on this but will/am now changeing joker back to your initial design (almost :D) and moving to as<> thingies...will see how it goes

I needed something regular for the other algorithms and shape based operation to work with less complexity.

it made my 'stuff' non-trivially more complex (still haven't cleaned up all the hacks around it) - i guess will have to do a live session to sort it out

Yeah as I don't have a view on your use cases, I can't take them into account.
The last batch of changes was to accomodate the variadic nature of shape and stride and do the heterogeneous requirements. It has been done so that the thing was comprehensible and had a regular, predictible behavior. So, now, if your specs changed or isn't aligned with that, I need some more informations or we'll be chasing aroudn each other for years and we both have other stuff on our respective plates.

jfalcou · 2023-08-01T13:55:58Z

include/kwk/detail/sequence/prefilled.hpp

-    KWK_TRIVIAL constexpr auto operator[](std::convertible_to<std::size_t> auto i) const noexcept
-    requires(is_dynamic && static_size>0)
+    KWK_TRIVIAL constexpr auto& operator[](std::convertible_to<std::uint32_t> auto i) noexcept
+    requires(is_fully_dynamic && static_size>0)
    {


why the change ?

jfalcou · 2023-08-01T13:56:31Z

include/kwk/detail/sequence/prefilled.hpp

+    KWK_TRIVIAL constexpr auto operator[](std::convertible_to<std::uint32_t> auto i) const noexcept
+    requires(!(is_fully_dynamic && static_size>0))
+    {
+      return as_array()[i]; // rely on std::array bounds checking


well as_array was suboptimal. I thought this was geenrting better code.

i think the main issue it that the way it was implemented actually did not work (compile) so this was a quick-fix...we'll have to go through all this live i guess

in which scenario did it din't work ?

i think when you used it on a shape that is only is_dynamic but not fully_dynamic (as the original constraint specified): you would index into a kumi tuple internal array that was shorter in length then the shape you were 'randomly' accessing

OK I see, our tests are then incomplete.

jfalcou · 2023-08-01T13:56:48Z

include/kwk/settings/axis.hpp

@@ -27,7 +27,7 @@ namespace kwk::__
    using is_product_type = void;
    using axis_kind       = axis_<ID,joker>;
    using content_type    = Content;
-    using base_type       = std::int32_t;
+    using base_type       = std::uint16_t;


same remark that for joker

jfalcou · 2023-08-01T13:57:46Z

include/kwk/settings/axis.hpp

+    KWK_PURE explicit constexpr operator Content          () const noexcept { return value; }
+    KWK_PURE          constexpr          Content operator*() const noexcept { return value; }
+
+#ifndef _WIN32 // https://github.com/llvm/llvm-project/issues/49358 Missing [[no_unique_address]] support on Windows


can this be put in a macro in detail/abi.hpp ?
Also is it really necessary as axis are mainly used as constexpr entity ?

i want the axis thingy to behave as a 'unit' wrapper (like boost.units)...wait for other changes..

include/kwk/utility/container/shape.hpp

…(kwk) wrappers].

Fixed map_descriptor to support/detect static_constants as static axes. Changed fixed<> to prevent recursive constant<constant<..>>s.

…ing KWK_DEFAULT_SHAPE_CONSTRAINTS_INCLUDE.

Added is_untyped and is_fully_typed attributes to prefilled. Added several shortcut implementations for is_fully_dynamic cases. Changed const prefilled<>::operator[] to return a 'typed axis' value. Changed/split/expanded prefilled<>::axis() to: axis_kind, axis_at, axis and axis_with_extent (preferring variable templates instead of functions). Fixed axis_::is_dynamic to support/handle static_constants. Added extent() as a thin alternative to as_dimension. Minor stylistic changes.

…ably moved to the enclosing namespace). Changed the shape(...) constructor to be explicit only for single-dim shapes (not necessary otherwise?). Similarily for differently sized conversion constructor: explicit only if requiring 'compression' (as that isn't obviously the behaviour everyone would want). Added a simple/direct operator== overload.

include/kwk/utility/joker.hpp

jfalcou · 2023-08-03T12:27:40Z

include/kwk/detail/kumi.hpp

@@ -71,6 +71,7 @@ namespace kumi::_
  {
    static constexpr bool is_homogeneous = true;
    T0 members[N];
+    constexpr bool operator==(binder_n const & other) const noexcept = default;


care to make a PR over at kumi ?

jfalcou · 2023-08-03T12:28:02Z

include/kwk/utility/traits/as_dimension.hpp

-  KWK_TRIVIAL constexpr auto as_dimension_(KWK_DELAY(), concepts::axis auto , std::integral auto d) { return d; }
-  KWK_TRIVIAL constexpr auto as_dimension_(KWK_DELAY(), std::integral auto v, std::integral auto  ) { return v; }
+  KWK_TRIVIAL constexpr auto as_dimension_(KWK_DELAY(), joker                , std::integral auto d) { return d; }
+  KWK_TRIVIAL constexpr auto as_dimension_(KWK_DELAY(), concepts::axis auto v, std::integral auto  ) { return v.value; }


well it was just wrong/incorrect as it was? as_dimension is supposed to return the 'extent' of 'v' is it not?
my code didn't work properly until i changed that

(why is as_dimension implemented with all this extra machinery is completely different question :D)

It passed its test and so was prefilled. So either I am missing some unit tests or somethign wrong was being called in your code.

purely from looking at the code/'logicwise': an axis has its value - why would you return some 'default' (d) instead of that (just like you return the 'left'/v value for integral and constant)

sometimes you don't have a value. You can have a shape<width,height> those are axis but has no value.

axis only has value when used in constexpr context like shape<my_axis[4]>.
At runtime, their value is passed via = as in shape x{ my_axis = 4}

Now this is something we may need to polish.

jfalcou · 2023-08-03T12:28:42Z

include/kwk/detail/sequence/prefilled.hpp

+  template <int index, typename... T>
+  using type_pack_element =
+#ifdef __clang__
+    __type_pack_element<index, T...>;


not a big fan of that

of what exactly

using the __type_pack thing in the wild

ok i'll add has_builtin to support old klangs and let joel sleep better ;P

also fill na issue to kumi

jfalcou · 2023-08-03T12:29:13Z

include/kwk/detail/sequence/prefilled.hpp

+    kumi::element_t<index, kumi::tuple<T...>>;
+#endif
+  template <int index, auto... NTTP>
+  using type_nttp_pack_element = type_pack_element<index, decltype(NTTP)...>;


this needs a bunch of explanations cause atm I don't get what you're trying to fix with this.

no fixing, was just extracting a repeated construct and using a faster/compiler builtin impl for it

Per visioconf at 09/09 : This is somethign we want as a factoring tool.
TODO: betetr name + better location

jfalcou · 2023-08-03T12:29:39Z

include/kwk/detail/sequence/prefilled.hpp

@@ -203,13 +236,6 @@ namespace kwk::__
      );
    }

-    // Static axis access
-    template<std::size_t N>
-    KWK_TRIVIAL constexpr auto axis() const noexcept


why is it removed ?

explained in the commit message: sort of i needed quite a bit more functionality regarding (static) axis access so i expanded/split this into more parts (plus wherever i could/got the chance changed things not to use constexpr functions but rather variable templates, should be faster to compile afaik) - sort of#2 to be discussed where to put all that

include/kwk/detail/sequence/shape_constraints.hpp

jfalcou · 2023-08-03T12:31:29Z

include/kwk/settings/axis.hpp

@@ -142,4 +155,24 @@ namespace kwk

  /// Predefined axis for channel
  inline constexpr auto channel = axis<"channel">;
+
+  // thin as_dimension
+         constexpr auto extent(std::integral             auto const dim) noexcept { return dim; }


why the duplication from as_dimension. Again, we tried ot get to a regular, straight forward setup before.
What are the rationale for these changes cause from afar it feels like ad hoc fixes to issue our use cases never encoutnered so I would appreciate a bit of backstory.

it was more like: what is this fat tag-invoke as_dimension thing with extra parameters that compiles longer and then gives me error messages like:
message : in instantiation of template class 'kwk::shape<decltype(kwk::tag_invoke(*this, e)){}, decltype(kwk::tag_invoke(*this, e)){}, 3ULL, 5ULL> :D

Again, we tried ot get to a regular, straight forward setup before.

don't know what you mean by this?

so I would appreciate a bit of backstory.

sure, right, as i said i was/am expecting that we'll have to have at least a short live sync

Ping me on discord so we can setup a date for this live sync :D
I am sure your doing the thing that needs to be done but from afar it is complicated to get the big picture.

The CPO system was meant to prevent issues I had in other options handling where specialisation were not found correctly.
Now, if the specialisation on those functions are not really needed becasue nobody will add new extent, we can shrink back to specific functions.

jfalcou · 2023-08-03T12:32:09Z

include/kwk/utility/container/stride.hpp

@@ -121,7 +121,7 @@ namespace kwk
  template<auto... D>
  KWK_PURE constexpr auto as_stride(shape<D...> s) noexcept
  {
-    if constexpr(sizeof...(D) == 1) return stride{s.template axis<0>() = fixed<1>};
+    if constexpr(sizeof...(D) == 1) return stride{s.template axis_with_extent<0>(fixed<1>)};


why the change ?

well the old axis<>() is no more (see above), now we have specialized utilites :D
and this axis_with_extent imo better conveys what you are trying to do/more readable (i use this, axis_with_extent, often in my code)

…e/static preprocessor based configuration point (use as<> otherwise). Protected __type_pack_element usage with a has_builtin check. Moved main/basic extent() functions to the extent.hpp header. Removed no longer needed base_type and is_joker axis_ members. Optimized fixed and axis_::is_implicit not to invoke CT function.

psiha · 2023-08-03T15:39:03Z

ps. shapes with mixed 'integral' and 'typed'/'named' axes (e.g. shape< 3, width >) did not compile for me on master, but do in my branch/fork

psiha · 2023-08-03T15:52:59Z

also the 'targets' for containers and docs in the cmake generated kiwaku vs solution do not compile (but unit tests do)

jfalcou · 2023-08-03T18:41:09Z

ps. shapes with mixed 'integral' and 'typed'/'named' axes (e.g. shape< 3, width >) did not compile for me on master, but do in my branch/fork

They probably compiles if they are properly setup afterward. Look at the doc on the website it has some explanation.
Here is a sample of what we accept as construction: https://godbolt.org/z/fxqa8o3Pd
Commented lines either fails to compiles or raise an assertion.

psiha force-pushed the main branch from a3e9453 to 1f38828 Compare May 8, 2023 12:08

jfalcou reviewed May 8, 2023

View reviewed changes

include/kwk/detail/kumi.hpp Outdated Show resolved Hide resolved

include/kwk/utility/container/stride.hpp Outdated Show resolved Hide resolved

include/kwk/utility/container/stride.hpp Outdated Show resolved Hide resolved

test/tts.hpp Show resolved Hide resolved

Domagoj Šarić added 8 commits July 18, 2023 09:57

Clang-CL: (quick) fixed compile-time option settings.

ce227a4

Clang-CL: workaround for static-order-initialization--fiasco in TTS.

61f780a

Fixed a -Wsign-conversion warning.

f308152

'Some change' (I guess in kumi) prevents shape from being treated as …

ac687e3

…trivial by automatically.

Quick-hack fix for MB: we need default axis type to be 16 bits (a pro…

c7486c8

…per fix would be to make this configurable :grimace:).

Fixed coordinates() to use the correct underlying std::array type.

a0a1066

Quick-hack fix for MB: we need default axis type to be 16 bits (a pro…

c969e1d

…per fix would be to make this configurable :grimace:).

psiha force-pushed the main branch from 592250e to c969e1d Compare July 19, 2023 10:32

Domagoj Šarić added 5 commits July 19, 2023 13:15

prefilled::get disambiguation.

ac9cbb5

Minor stylistic fixes.

fb4f448

Hack-fix for compilation errors in coordinates() for shapes created w…

76f2aae

…ith of_size() (which uses to_descriptor() - why?).

axis_:

902560d

- changed base_type to uint16 to match the (new) joker default (awaiting a proper solution) - added conversion operators to Content/value - attempt to make axis_ w/ empty Content an empty class.

Fixed warning-as-error on non-Clang compilers.

85af75c

psiha force-pushed the main branch from d9855a9 to 85af75c Compare July 22, 2023 07:04

jfalcou requested changes Aug 1, 2023

View reviewed changes

psiha changed the title ~~Further help performance of indexing code in debug and sanitizer-enab…~~ MB hacks, required changes and brainstorming for latest master Aug 2, 2023

Domagoj Šarić added 6 commits August 3, 2023 13:09

Fixed as_dimension for axis.

686e177

Add operator== to tuple [to enable easier and simpler equivalents in …

1e76764

…(kwk) wrappers].

Ad-hoc ability to configure underlying joker integral type (its size).

87ad842

Fixed map_descriptor to support/detect static_constants as static axes. Changed fixed<> to prevent recursive constant<constant<..>>s.

'Fixed' KWK_DEFAULT_SHAPE_CONSTRAINTS support by adding the accompany…

5bc8bf7

…ing KWK_DEFAULT_SHAPE_CONSTRAINTS_INCLUDE.

jfalcou reviewed Aug 3, 2023

View reviewed changes

include/kwk/utility/joker.hpp Outdated Show resolved Hide resolved

jfalcou reviewed Aug 3, 2023

View reviewed changes

include/kwk/detail/sequence/shape_constraints.hpp Outdated Show resolved Hide resolved

jfalcou reviewed Aug 3, 2023

View reviewed changes

Domagoj Šarić added 4 commits August 3, 2023 16:37

New shiny CMake objects.

d27c852

Simplified KWK_DEFAULT_SHAPE* logic.

6b3271d

Updated the unit tests with the latest changes.

01fa3e5

Domagoj Šarić added 3 commits August 3, 2023 20:24

Made constant usable in assume statements w/o compiler warnings.

dd4c6e1

Ditto extent.

3649d35

Missing axis_with_extent for type_info.

0c34146

Domagoj Šarić and others added 4 commits August 3, 2023 21:39

Fixed (hopefully) axis == integral and joker comparisons.

6afa7f7

Properly fixed the issue of shape<> not being trivially copyable.

f4cd4ed

use std::equality_comparable_with which is now fully supported

07af4bd

fix narrowing cast

72e45da

MB hacks, required changes and brainstorming for latest master #115

Are you sure you want to change the base?

MB hacks, required changes and brainstorming for latest master #115

Conversation

psiha commented Apr 11, 2023

jfalcou 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psiha Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

psiha commented Aug 3, 2023

psiha commented Aug 3, 2023

jfalcou commented Aug 3, 2023

psiha Aug 3, 2023 •

edited

Loading