[Mesa-dev] [PATCH (resend)] r600/sb: Don't require array declarations for TGSI_FILE_SYSTEM_VALUE

[Roland Scheidegger]

Am 02.02.2018 um 07:53 schrieb Gert Wollny:
> Am Freitag, den 02.02.2018, 02:13 +0100 schrieb Roland Scheidegger:
>> Am 30.01.2018 um 09:21 schrieb Gert Wollny:
>>> Although gl_SampleMaskIn is declared as an array in GLSL, it is
>>> effectively a 32 bit mask on all hardware supported by mesa, so the
>>> array indexing is ignored (Thanks Glenn Kennard for the
>>> explanation).
>>>
>>> Add a comment that the assert is not made superfluos by the else
>>> branch.
>>>
>>> Corrects: piglit spec at arb_gpu_shader5@execution at samplemaskin-
>>> indirect for
>>> debug builds (it already passed in release).
>>>
>>> Signed-off-by: Gert Wollny <gw.fossdev at gmail.com>
>>> ---
>>>
>>>  src/gallium/drivers/r600/sb/sb_bc_parser.cpp | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
>>> b/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
>>> index 970e4141d5..b92e058daf 100644
>>> --- a/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
>>> +++ b/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
>>> @@ -125,7 +125,9 @@ int bc_parser::parse_decls() {
>>>  		return 0;
>>>  	}
>>>  
>>> -	if (pshader->indirect_files & ~((1 << TGSI_FILE_CONSTANT)
>>> | (1 << TGSI_FILE_SAMPLER))) {
>>> +	if (pshader->indirect_files &
>>> +	    ~((1 << TGSI_FILE_CONSTANT) | (1 << TGSI_FILE_SAMPLER)
>>> |
>>> +          (1 << TGSI_FILE_SYSTEM_VALUE))) {
>>
>> So I suppose this is ok because none of the other system values
>> could be arrays.
>> That said, isn't that actually a bug in glsl to tgsi translation? the
>> sample mask is never an array in gallium/tgsi (and not declared as
>> such), so I think it should not turn up as indirect when properly
>> translated, reading the value should not end up as indexed.
> 
> Well, the spec says the sample mask is an array [1], but Glenn told me
> that in the end on all supported hardware it ends up as a bit mask [2].

Yes, the _GL spec_ says it is an array.
But in gallium it can't be. Therefore I think it's incorrect if we end
up with array accesses there (albeit I was too lazy to actually look at
the tgsi, but I'm pretty sure it isn't declard as an array).

Roland

> Best, 
> Gert 
> 
> [1] 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.khronos.org_registry_OpenGL-2DRefpages_gl4_html_gl-5FSampleMask&d=DwICaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=FcYHgyjoyYUywC54bIyEgOTUo_kmJWgEGLt49fTYkO4&s=r1U2A6MGHIWSsd2qcriwKNcZYFcjPKBvUA97L3O58jw&e=
> .xhtml
> 
> [2]https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_mesa-2Ddev_2017-2DSeptember_16937&d=DwICaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=FcYHgyjoyYUywC54bIyEgOTUo_kmJWgEGLt49fTYkO4&s=xzMOlOUwz2OklnZ9QIEl_bezbIYRcvb0KqMfWuj3MMA&e=
> 4.html
>>
>> Roland
>>
>>
>>>  		assert(pshader->num_arrays);
>>>  
>>> @@ -135,6 +137,10 @@ int bc_parser::parse_decls() {
>>>  				sh->add_gpr_array(a.gpr_start,
>>> a.gpr_count, a.comp_mask);
>>>  			}
>>>  		} else {
>>> +			/* When the above assert is disabled and
>>> proper array info
>>> +			 * is missing for some reason then, as a
>>> fallback, make sure
>>> +			 * that all GPRs can be accessed
>>> indirectly.
>>> +			 */
>>>  			sh->add_gpr_array(0, pshader->bc.ngpr,
>>> 0x0F);
>>>  		}
>>>  	}
>>>
>>
>>