Re: [eigen] eigen short support: discussion thread

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2009/8/29 Rohit Garg <rpg.314@xxxxxxxxx>:
> On Sat, Aug 29, 2009 at 10:42 PM, Benoit Jacob<jacob.benoit.1@xxxxxxxxx> wrote:
>> 2009/8/29 Rohit Garg <rpg.314@xxxxxxxxx>:
>>> 1) With regard to the __m128i being the common sse type for short
>>> vectors and int vectors, how about we let just the Packet8s be a
>>> typedef for a struct/class containing a __m128i. This way, they'll be
>>> different types.
>>>
>>> I want to ask if people here think this would work and if it will be
>>> cheaper to implement than other ways?
>>
>> Actually, you raise a very good point. The solution that I proposed
>> looks like this except that I only proposed to make such a struct wrap
>> the packet type, not a packet object. I proposed an empty struct. Now
>> it's tru that when we want to implement e.g. ei_padd, it looks like
>>
>> packet_type ei_padd(const packet_type&, const packet_type&)
>>
>> and so if we want " z = ei_padd(x,y) " to work, we need packet_type to
>> actually contain an object, not just a typedef. My solution would have
>> forced one to pass a "Scalar" template parameter to all the ei_p...
>> methods taking packets -- since these packet _objects_ still didn't
>> remember about the scalar type.
>>
>> So, +1 to your idea. Gael?
>
> I assume that this means we can just copy-paste int-to-short routines
> (for the most part) then. :)  I am interested in avoiding changes
> outside PacketMath.h where ever possible.

Yes, indeed this solution is probably very local in the sense that it
doesn't require adjustments outside of PacketMath.h. that's good.

>
>> Your struct could be called ei_packet. It remains to be seen if
>> ei_packet_traits can be merged into it, or not.
>>
>>> 2) What extra needs to be done for the basic short support to be
>>> merged into eigen mainline? There's a unit test (a simple one) in my
>>> branch.
>>>
>>> http://bitbucket.org/rpg/eigen2-rpg-branch/src/tip/test/short.cpp
>>>
>>> Is it enough for the code to be commited? If not, what else is
>>> missing? (vectorization will obviously take a bit more time)
>>
>> First, are you sure that this should go into Core? Why not make this a
>> separate module? It could go directly into Eigen/ skipping
>> unsupported/ as it's obvious that there are many users interested in
>> this.
>>
>> I'd rather have this as a separate module (not a very strong opinion
>> though) and I don't remember a decision on this being made, perhaps
>> this is something you already discussed with Gael?
> Since we are adding types, and not functions like cwise(), QR, SVD
> etc., I'd prefer to have it in Core, merely for the sake of
> consistency.

I never regarded that as being the criterion for deciding what goes
into Core and what's split as separate modules.

In an ideal world, we wouldn't have multiple modules. The reasons why
we split stuff into separate modules, are:
0) limit code complexity (and sheer length) in Core
1) limit compilation times
2) limit compiler memory usage
3) limit triggering of compiler errors

it's a matter of "you only pay for what you use". In the case of short
integer types support, my main fears are with 0) and 3). 0) because
PacketMath.h is already scary enough like this, and will already have
to become a little scarier with the change we're discussing
(ei_packet); 3) because if you have to do lowlevel stuff, perhaps
dependent on the SSE version, on the compiler, etc, we'll have a lot
of code paths to check for compiler errors (missing intrinsics, etc)
and i can sense danger here. By having this in a separate module, we
make sure that any compiler errors only happen for people who at least
are interested in that functionality.

Also keep in mind that it's not clear yet how many small integer types
we'll have to support. If it were only short then yes it'd go into
Core. But eventually it'll probably be:

int8
uint8
int16
uint16

and we'll quite possibly feel compelled to add uint32 too for
consistency. By the way, what about int64 vectorization? Sooner or
later, someone will probably add this. So perhaps it'll be an
"IntegerTypes" rather than "SmallIntegerTypes" module eventually? And
move the current "long long" stuff there too?

That would be no less than 7 integer types (in addition to the int
that we have in Core) so having these 7 types in Core would more than
double the number of types handled in PacketMath.h ---> i think that
warrants the creation of a new module.

I should have presented this as a "scalability" argument from the onset...

>> When you do vectorization, add also packetmath.cpp. Keep all that
>> templated, so it can easily be extended to cover more new small
>> integer types. Just wipe what is not wanted / not applicable for small
>> integer types.
> Actually, I have looked at char and uchar support in SSE2 as well.
> Let's just say that we have a SNAFU (thank you, Intel, :P ) here.

heh, i can believe it's not going to be easy!

>> Do you want write access to eigen2 so you can eventually push
>> yourself? (nontrivial additions into Core, especially, still need to
>> be discussed on this list)
> Well, not yet. I'd rather have a complete set of unit-tests before pushing.

ok, tell me when you need write access.

Benoit