jfalcou / kiwaku

C++20 and onward collection of high performance data containers and related tools
https://jfalcou.github.io/kiwaku
Boost Software License 1.0
51 stars 5 forks source link

Configurable dimension type #6

Closed psiha closed 2 years ago

psiha commented 2 years ago

For example 16bit ints are quite enough for our use case and an array of 4 shorts (for 4d arrays - typical "tensors" used in ML) fits into a single 64bit register - so a fully dynamic view can be passed by value with only two 64bit registers (as opposed to five if you use size_ts)...

jfalcou commented 2 years ago

Good point. We ponderef about this but wzrent sure of the utility. Glad to have confirmation.

We'll see where we can pass this information

jfalcou commented 2 years ago

Updated API being developed in #13 :

auto s1  = kwk::extent(1,2,3); // [1 2 3] as a shape stored in ptrdiff_t
auto s2  = kwk::extent(1,kwk::fixed<2>,3); // [1 2 3] as a shape stored in ptrdiff_t but 2 is compile-time info
auto s2b = kwk::extent(1,2_c,3); // Same but use the _c literal suffix
auto s3  = kwk::extent_of<short>(1,2,3); // [1 2 3] as a shape stored as short

The old shape<extent()[4]>is now shape<shape_()[4]> but only generates ptrdiff_t based shape storage. I wen against deducing the storage type for shape based on the type of w/e it is passed as argument cause it would lead to subtle bugs and writing short(4) everywhere seemed bad.

extent_of<T> is the best I found atm. If you have a better naming idea, I'm open to hearing it.

Sample codegen:

auto f(short n) {  return kwk::extent_of<short>(n,2*n); }

becomes:

f(short):                                  # @f(short)
        movzwl  %di, %eax
        shll    $17, %edi
        orl     %edi, %eax
        retq
psiha commented 2 years ago

Great for the fast move forward πŸ‘ However why the move away from the funky ()[]()[][] syntax? The new way seems more like the old days of proto and placeholders...is there a big compile time difference?

extent_of is the best I found atm. If you have a better naming idea, I'm open to hearing it.

  • Perhaps you don't need a new name/function template - perhaps extent can also be parameterized on T just have it defaulted to ptrdiff_t? (I guess it's probably a variadic template and it is impossible but try and consider it first)
  • Also, it's not an either-or: you can have both the explicit T and automatic deduction (if no T is specified) which deduces to the largest type passed (std::common_type) - this would work nicely with runtime/dynamic values stored shape_t = uint16_t aliased variables ... but I understand that it may knife compile time performace 😊 unless you have some more cpp20 magic up your sleave 😁
jfalcou commented 2 years ago

It's not a move away, extent is just a function that computes the correct shape_ types without users having to deal with it.

My first idea was to have:

shape< shape_<short>()[4] > x;
shape< shape_()[4] > y;

but shape_ being a template variable, it can't exist in both ways.

The function is suffering from the following shortcomings. Say we have just extent(Dims...) and extent(Dims...). When you pass a single value, the call is ambiguous on some compilers. But this is something I can probably fix, thus not needing two names indeed.

Good catch on the common_type, something like using common_type<T...>::type is maybe a good idea.

jfalcou commented 2 years ago

Yeah, OK, fixed the naming thing. I had fatty fingers and wrote crap.

psiha commented 2 years ago

It's not a move away, extent is just a function that computes the correct shape_ types without users having to deal with it.

hm so let's see if i follow it correctly now:

And now you have just

shape<extent()[4]>is now shape<shape_()[4]>

renamed extent to shape_? If so - why? πŸ˜‚

jfalcou commented 2 years ago

I need multiple things:

jfalcou commented 2 years ago

Also note, than before any v0.1 tagged release, we're tryign to find the better naming/setup before freezing the API.

psiha commented 2 years ago

great .. except shape_ (anything with an underscore) looks ugly/hackish/'workaroundish' (and you have to very similarily named things next to each other)...do not see why extent would be inconsistent .. i prefer extent, but hey, a minor issue..

jfalcou commented 2 years ago

Yeah I know. Again I may find a better name

jfalcou commented 2 years ago

OK, mulled over the nameing scheme. We're back to shape/extent[]()/ and ofsize for the functional generator. No more ugly shape

jfalcou commented 2 years ago

Fixed in 32d881a072922c193d2a4d8bc1a92ddbcb807cb3

psiha commented 2 years ago

so how do i do this in the end? 😁 extent does not support the underlying-type template argument? i have to use detail::shaper or?

jfalcou commented 2 years ago

https://github.com/jfalcou/kiwaku/blob/main/test/container/shape/mixed.cpp

For how to make of_size change types.

Do you need an helper for the type or ?

jfalcou commented 2 years ago

Ok I see what is missing.

psiha commented 2 years ago

tried shape< extent< short >()()()() > but that does not work obviously - i need to use of_size for that?

jfalcou commented 2 years ago

You can but wait a bit, I am in the car ATM. Will push something tonight.

jfalcou commented 2 years ago

Ok so here are the options once the new PR lands:

#include<kwk/container/shape.hpp>

// Shape descriptors with runtime sizes
kwk::shape< kwk::extent()()           > d1(6, 8); // use ptrdiff_t
kwk::shape< kwk::extent_of<short>()() > s1(6, 8); // use short

// Shape with of_size
auto p1 = kwk::of_size(4,4); // use int internally (because 4 is int)
auto q1 = kwk::of_size<short>(4,4); // use short

short n = 8, m = 7;
auto r1 = kwk::of_size(n,m); // use short
jfalcou commented 2 years ago

Merged.

psiha commented 2 years ago

are you sure this works with of_size? i'm getting:

error : call to 'of_size' is ambiguous kiwaku\include\kwk/container/shape.hpp(544,18): message : candidate function [with SizeType = unsigned short, Ds = <unsigned short, unsigned short, std::integral_constant<unsigned char, '\x01'>>] kiwaku\include\kwk/container/shape.hpp(570,43): message : candidate function [with Ds = <unsigned short, unsigned short, std::integral_constant<unsigned char, '\x01'>>]

jfalcou commented 2 years ago

Can you show me your call exactly ?

psiha commented 2 years ago

Can you show me your call exactly ?

bit complicated

template < auto ... dims >
//struct Dimensions : kwk::shape< kwk::detail::shaper< shape_t, KWKType< dims >... >{ dims.size... } > {}; // this does not compile - the new constructor issue
struct Dimensions : kwk::shape< makeShaper< dims ... >( kwk::extent_of< shape_t > ) >
{
...
    template < auto ... targetDims >
    constexpr operator Dimensions< targetDims... >() const noexcept requires( equivalent( Dimensions< targetDims... >{}, Dimensions< dims... >{} ) )
    {
        return {{ kwk::of_size< shape_t >( Detail::getDimForKWKCopyConstruction< targetDims >( *this )... ) }};
    }
...
};

(implementing/experimenting layout support/typed dimensions locally😊)

If I comment out the overload of of_size w/o SizeType it compiles...

jfalcou commented 2 years ago

kiwaku\include\kwk/container/shape.hpp(544,18): message : candidate function [with SizeType = unsigned short, Ds = <unsigned short, unsigned short, std::integral_constant<unsigned char, '\x01'>>] kiwaku\include\kwk/container/shape.hpp(570,43): message : candidate function [with Ds = <unsigned short, unsigned short, std::integral_constant<unsigned char, '\x01'>>]

so this means you do a of_size<unsigned short>(us,1_c) ?

jfalcou commented 2 years ago

OK reproduced.

jfalcou commented 2 years ago

OK, I guess I was a bit optimistic. There is some ambiguity in the current code. Are with ok with having a of_size and a of_size_with<T> ?

psiha commented 2 years ago

OK, I guess I was a bit optimistic. There is some ambiguity in the current code. Are with ok with having a of_size and a of_size_with<T> ?

oh well...let's go with that for starters :)

jfalcou commented 2 years ago

it can also be a parameters if you want

of_size(type<T>, ds...)

jfalcou commented 2 years ago

So alternative works:

of_size(as<short>, v1, v2);

which one do you prefer ?

psiha commented 2 years ago

which one do you prefer ?

of_size_with (at least this should compile faster than additional type wrappers:)

jfalcou commented 2 years ago

:+1: Shipping away in a few mn

jfalcou commented 2 years ago

Thanks to discord hivemind, I actually fixed the original issue.

template<typename SizeType, int..., typename... Ds>
  constexpr auto of_size(Ds... ds) noexcept
  {
    return whatever;
  }

  template<int..., typename... Ds> constexpr auto of_size( Ds... ds) noexcept
  {
    using type_t = typename detail::largest_type<detail::to_int_t<Ds>...>::type;
    return of_size<type_t>(ds...);
  }

is enough to make of_size(d...) and of_size<T>(d...) to be non ambiguous

jfalcou commented 2 years ago

See: https://github.com/jfalcou/kiwaku/pull/30

jfalcou commented 2 years ago

30 has been merged. If you can check on your side it's OK.

I added the use case to the unit tests though.

psiha commented 2 years ago

this works now πŸ‘ now waiting for https://github.com/jfalcou/kiwaku/issues/26 😁