[Nouveau] [PATCH] nouveau: codegen: Take src swizzle into account on loads

[Hans de Goede]

Hi,

On 08-04-16 17:45, Ilia Mirkin wrote:
> On Fri, Apr 8, 2016 at 11:28 AM, Hans de Goede <hdegoede at redhat.com> wrote:
>> When dealing with non vector variables the llvm register allocator
>> will use TEMP[0].x then TEMP[0].y, etc.
>>
>> When loading something from a global buffer it will calculate the
>> address to use, and store that in say TEMP[0].x, so it ends up
>> generating:
>>
>> LOAD TEMP[0].y, MEMORY[0], TEMP[0]
>>
>> Expecting the contents of TEMP[0].y to become the 32 bits of data
>> to which TEMP[0].x is pointing. But instead it will get the 32 bits of
>> data at address (TEMP[0].x + 4).
>>
>> With the old RES[32767] code one could generate the following TGSI:
>>
>> LOAD TEMP[0].y, RES[32767].xxxx, TEMP[0]
>>
>> And things would work fine since the .xxxx swizzling postfix would
>> be honored and when storing to y (the only component set in the dest-mask)
>> the x component at address (TEMP[0].x) would be loaded, rather then the
>> y component at (TEMP[0].y)
>>
>> Note that another approach would be to not increment the address by
>> a 32 bit word for skipped (not set in destmask) components.
>>
>> The way I see it either:
>>
>> 1) We see that LOAD does not deal with vectors, but with flat memory,
>> in which case skipping 4 bytes because x is not set in the destmask
>> does not make sense, as that is a vector thing todo.
>>
>> 2) LOAD is vector layout aware in which case supporting swizzling
>> makes sense.
>>
>> Currently we have a weird hybrid which is rather cumbersome to
>> work with from a compiler pov.
>
> And I guess LLVM never ends up generating any of the other "funny"
> instructions like LIT and the such. Well, I have no problem adding the
> swizzling logic, i.e. the way that LOAD will now work (logically) is
> that it will
>
> (a) fetch 4 values from the coordinates provided (4 sequential dwords
> from src1.x in the case of buffer/memory, RGBA colors from src1.xyz in
> the case of images)
> (b) swizzle them according to the swizzle on the MEMORY/BUFFER/IMAGE argument
> (c) store that swizzled result into the destination based on the writemask
>
> That would sound reasonable to me, and if I understand correctly, is
> option 2 of your proposal.

Yes that is option 2, and is basically what the patch which started this
thread does. So that would work for me :)

> We'd need some docs updates and buy-in from the other gallium driver developers.

What docs would need updating ? The TGSI docs I'm aware of are at:

http://gallium.readthedocs.org/en/latest/tgsi.html

I assume those have a source in the mesa src somewhere (I've not looked),
but those mostly just look quite incomplete in general when it comes to TGSI
(I've had to revert to figuring what things do from the mesa srcs quite often)

Have I been looking at the wrong docs perhaps ?

Note that them being incomplete is not intended as an excuse to not document
this, I'm all for better documentation.

> STORE remains unchanged, as the MEMORY/etc is in the destination,
> where there is a writemask, which is presently used and will remain
> effective.

Right and note that the first src operand of STORE already takes swizzling
into account, so the proposed option 2 will actually make the 2 more inline.

Regards,

Hans