[Mesa-dev] [PATCH 9/9] tgsi: add ArrayID documentation

[Christian König]

Am 15.03.2013 18:58, schrieb Christoph Bumiller:
> On 15.03.2013 13:08, Christian König wrote:
>> Am 14.03.2013 15:53, schrieb Christoph Bumiller:
>>> On 14.03.2013 15:20, Christian König wrote:
>>>> From: Christian König <christian.koenig at amd.com>
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>    src/gallium/docs/source/tgsi.rst |   16 ++++++++++++++++
>>>>    1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/src/gallium/docs/source/tgsi.rst
>>>> b/src/gallium/docs/source/tgsi.rst
>>>> index d9a7fe9..27fe039 100644
>>>> --- a/src/gallium/docs/source/tgsi.rst
>>>> +++ b/src/gallium/docs/source/tgsi.rst
>>>> @@ -1833,6 +1833,22 @@ If Interpolate flag is set to 1, a
>>>> Declaration Interpolate token follows.
>>>>      If file is TGSI_FILE_RESOURCE, a Declaration Resource token
>>>> follows.
>>>>    +If Array flag is set to 1, a Declaration Array token follows.
>>>> +
>>>> +Array Declaration
>>>> +^^^^^^^^^^^^^^^^^^^^^^^^
>>>> +
>>>> +Declarations can optional have an ArrayID attribute which can be
>>>> referred by
>>>> +indirect addressing operands. An ArrayID of zero is reserved and
>>>> treaded as
>>>> +if no ArrayID is specified.
>>>> +
>>>> +If an indirect addressing operand refers to an specific declaration
>>>> by using
>>> s/an/a
>> Thx, fixed.
>>
>>>> +an ArrayID only the registers in this declaration are guaranteed to be
>>>> +accessed, accessing any register outside this declaration results
>>>> in undefined
>>>> +behavior.
>>> + Note that the effective index is zero-based and not relative to the
>>> specified declaration. XXX: Is it ? Should it be ?
>> Yes for compatibility reasons, otherwise we would need to change all
>> drivers at once.
>>
>>>> +
>>>> +If no ArrayID is specified with an indirect addressing operand the
>>>> whole
>>>> +register file might be accessed by this operand.
>>>>    
>>> + A practice which is strongly discouraged. Don't do this if you have
>>> more than 1 declaration for the file in question ! It will prevent
>>> packing of scalar/vec2 arrays and effective memory alias analysis.
>> A bit shortened, but in general added the remark.
>>
>>> Packing ? Yes !
>>> We can pack arrays if they're declared as e.g.
>>> TEMP[0-3].xyzw
>>> TEMP[4-31].x
>>>
>>> And the caches will be very very thankful that we don't just access
>>> every 4th element of our 4 times larger than it needs to be buffer !!!
>>>
>>> And if your card can't do that, pleeease be nice and still make it
>>> possible for other drivers. :o3
>> It is probably possible with the new information to do so, but not
>> priority for me cause I primary need it for our LLVM backend.
>>
> At some point you'll be able to make use of the info in your backend,
> too, and then you'll regret having to refamiliarize with this code just
> because you didn't add the extra (estimated) 2 lines to set the UsageMask.

I think you misunderstood me here, you don't need the UsageMask to 
generate those informations. It is possible by just scanning the shader 
to figure out which channels are used and which aren't.

Additional to that I'm not convinced that using the UsageMask for this 
is 100% correct, to me it looks more like UsageMask is something we need 
for outputs to distinct between not writing to an output channel (and so 
still having the default) and not having an output channel at all.

> Also, NAK from me until array access/declarations for the other files
> follows suit.
> Sorry for being so ... pesky, but I'd really like this change to be 100%
> complete. Come on, doesn't it nag on your conscience if this is left to
> remain only a few smalls steps from perfection ?

Declaring and accessing arrays for inputs/outputs are not so much of a 
problem, figuring out how to get this information to glsl_to_tgsi is the 
real problem. For temporaries changing the glsl_to_tgsi pass is pretty 
much sufficient, but for inputs and outputs you need to dig into the 
mesa state tracker, and I definitely don't intend to do so.

Christian.