Re: [PATCH 5/9] powerpc: Introduce VSX thread_struct and CONFIG_VSX

[Kumar Gala <galak@kernel.crashing.org>]

On Jun 19, 2008, at 1:01 AM, Michael Neuling wrote:

> In message <B0E87874-BC65-4037- 
> A43D-91C4142475E7@kernel.crashing.org> you wrote
> :
>>>>>>> +	} fpvsr __attribute__((aligned(16)));
>>>>>>
>>>>>> Do we really need a union here?  what would happen if you just
>>>>>> changed
>>>>>> the type of fpr[32] from double to vector if #CONFIG_VSX?
>>>>>>
>>>>>> I really dont like the union and think we can just make the  
>>>>>> storage
>>>>>> look opaque which is the key.  I doubt we every really care about
>>>>>> using fpr[] as a double in the kernel.
>>>>>
>>>>> I did something similar to this for the first cut of this patch,  
>>>>> but
>>>>> it
>>>>> made the code accessing this structure much less readable.
>>>>
>>>> really, what code is that?
>>>
>>> Any code that has to read/write the top or bottom 64 bits _only_ of
>>> the
>>> 128 bit vector.
>>>
>>> The signals code is a good example where, for backwards  
>>> compatibility,
>>> we need to read/write the old 64 bit FP regs, from the 128 bit value
>>> in
>>> the struct.
>>>
>>> Similarly, the way we've extended the signals interface for VSX, you
>>> need to read/write out the bottom 64 bits (vsrlow) of a 128 bit  
>>> value.
>>>
>>> eg. the simple:
>>>    current->thread.fpvsr.fp[i].vsrlow = buf[i]
>>> would turn into some abomination/macro.
>>
>> it would turn into something like:
>>
>> current->thread.fpr[i][2] = buf[i];
>> current->thread.fpr[i][3] = buf[i+1];
>
> Maybe abomination was going too far :-)
>
> I still think using the union makes it is easier to read than what you
> have here.  Also, it better reflects the structure of what's being
> stored there.

I don't think that holds much weight with me.  We don't union the  
vector128 type to show it also supports float, u16, and u8 types.

I stick by the fact that the ONLY place it looks like you access the  
union via the .vsr member is for memset or memcpy so you clearly know  
if the size should be sizeof(double) or sizeof(vector).

Also, I can see the case in the future that 'fpr's become 128-bits  
wide' and allow for native long double support.

- k