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

[Christoph Bumiller]

On 17.03.2013 16:30, Christian König wrote:
> 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.
>
For temporaries that may be true ... and inputs/outputs are always vec4
sized to guarantee linkage, packing for GENERIC ones is handled at the
mesa level.

> 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.
>
Actually, for gl_ClipDistance[] we use the UsageMask to specify if the
clip distance was declared in the source (and thus should be enabled)
instead of whether it's been written or not.

I wanted to be able to distinguish between
float gl_ClipDistance[8] or
vec4 mesa_ClipDistance[2] with the UsageMask but I guess

OUT[0..1].x, CLIPDIST might just as well mean that gl_ClipDistance[0 and
4] are being used ...

Hm, we'll need a cap for that anway to tell st if it should lower
ClipDistance to vec4s or not, and just assume that TGSI corresponds to
what the cap says.
And since this is the only case for IN/OUT where the driver's backend
has to decide whether to pack or not ... ok, I'll just infer array width
myself, too, and you can ignore the UsageMask.

>> 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.
>
Fine, then I'll have a look at that myself. * flaming eyes *

> Christian.