Re: [eigen] eigen short support: discussion thread

[Rohit Garg <rpg.314@xxxxxxxxx>]

On Sat, Aug 29, 2009 at 11:22 PM, Benoit Jacob<jacob.benoit.1@xxxxxxxxx> wrote:
> 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.

That's encouraging.
>
>>
>>> 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?

It wont be a complete integer types module without taking int32 there.
And that might break API/ABI. (Not sure, just guessing). How about
making it a sub module of Core?
>
> 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!
It isn't about it being easy.

Let me try to list out the vector operations available in int8/uint8

int8

add,
sub,
saturated add, sub
and,or,not
==, <, > comparison

uint8
add with saturation
average
min
max
saturated add, sub
and, or, not
==, <, > comparison

That's all what is there in SSE2. No multiplication, no division, no
bit shifts. With these handicaps, is char/uchar even useful to
_anyone_ ?

>
>>> 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
>
>
>



-- 
Rohit Garg

http://rpg-314.blogspot.com/

Senior Undergraduate
Department of Physics
Indian Institute of Technology
Bombay