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

In message <B0E87874-BC65-4037- 
A43D-91C4142475E7@kernel.crashing.org> you wrote
:

quoted

quoted

quoted

quoted

quoted

quoted

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