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

[Christoph Bumiller]

On 17.03.2013 18:04, Christoph Bumiller wrote:
> 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 *

Ok, had a look, had enough. It hurts. At least if you have never touched
glsl-to-tgsi and go about it expecting it to be easy and straightforward.
HURTS!

>> Christian.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>