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

[Ilia Mirkin]

On Fri, Feb 2, 2018 at 7:55 AM, Gert Wollny  wrote:
> Am Freitag, den 02.02.2018, 06:56 -0500 schrieb Ilia Mirkin:
>> On Fri, Feb 2, 2018 at 4:07 AM, Gert Wollny 
>> wrote:
>> > Am Freitag, den 02.02.2018, 09:04 +0100 schrieb Roland Scheidegger:
>> > >
>> > >
>> > > 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).
>> >
>> > the TGSI for the relevant shader in the piglit looks like this:
>> >
>> > FRAG
>> > DCL SV[0], SAMPLEMASK
>> > DCL OUT[0], COLOR
>> > DCL CONST[0][0]
>> > DCL TEMP[0..1], LOCAL
>> > DCL ADDR[0]
>> > IMM[0] FLT32 {1., 0., 0., 0.}
>> > IMM[1] INT32 {1, 0, 0, 0}
>> >   0: MOV TEMP[0], IMM[0].xyyx
>> >   1: UARL ADDR[0].x, CONST[0][0].
>> >   2: USEQ TEMP[1].x, SV[ADDR[0].x]., IMM[1].
>>
>> OK, this is a big problem. I'm guessing the GLSL code was something
>> like
>>
>> gl_SampleMaskIn[uniform]
>>
> This is how the piglit is written, and the standard definess
> gl_SampleMaskIn is defined as an array, so this makes sense.
>
>> What this got translated into was an indirect access into the *global
>> implicit array of system values*. We don't want that. glsl_to_tgsi
>> should just be dropping the indirect access entirely.
> Just found this comment in mesa/st/st_glsl_to.tgsi.cpp:6495
>
>  "TODO: If we ever support more than 32 samples, this will have
>to become an array."
>
> which would imply to me that in this stage the indirect access might at
> one point become relevant.

Pretty sure I wrote that :) (commit c5d822dad90)

I added it as part of ARB_sample_shading which just has a
gl_SampleMask output in FS. I'm sure I didn't think about indirect
accesses at the time. (Not even sure they'd be legal on the output...
I guess probably would.)

To support indirect accesses, one would have to declare an arrayid,
probably -- either way, it'd be a whole thing to support it. For now,
it should be disallowed and that indirect arg should get dropped.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gert Wollny]

Am Freitag, den 02.02.2018, 06:56 -0500 schrieb Ilia Mirkin:
> On Fri, Feb 2, 2018 at 4:07 AM, Gert Wollny 
> wrote:
> > Am Freitag, den 02.02.2018, 09:04 +0100 schrieb Roland Scheidegger:
> > > 
> > > 
> > > 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).
> > 
> > the TGSI for the relevant shader in the piglit looks like this:
> > 
> > FRAG
> > DCL SV[0], SAMPLEMASK
> > DCL OUT[0], COLOR
> > DCL CONST[0][0]
> > DCL TEMP[0..1], LOCAL
> > DCL ADDR[0]
> > IMM[0] FLT32 {1., 0., 0., 0.}
> > IMM[1] INT32 {1, 0, 0, 0}
> >   0: MOV TEMP[0], IMM[0].xyyx
> >   1: UARL ADDR[0].x, CONST[0][0].
> >   2: USEQ TEMP[1].x, SV[ADDR[0].x]., IMM[1].
> 
> OK, this is a big problem. I'm guessing the GLSL code was something
> like
> 
> gl_SampleMaskIn[uniform]
> 
This is how the piglit is written, and the standard definess
gl_SampleMaskIn is defined as an array, so this makes sense. 

> What this got translated into was an indirect access into the *global
> implicit array of system values*. We don't want that. glsl_to_tgsi
> should just be dropping the indirect access entirely.
Just found this comment in mesa/st/st_glsl_to.tgsi.cpp:6495

 "TODO: If we ever support more than 32 samples, this will have
   to become an array." 

which would imply to me that in this stage the indirect access might at
one point become relevant. 
   
Best, 
Gert
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ilia Mirkin]

On Fri, Feb 2, 2018 at 4:07 AM, Gert Wollny  wrote:
> Am Freitag, den 02.02.2018, 09:04 +0100 schrieb Roland Scheidegger:
>>
>>
>> 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).
> the TGSI for the relevant shader in the piglit looks like this:
>
> FRAG
> DCL SV[0], SAMPLEMASK
> DCL OUT[0], COLOR
> DCL CONST[0][0]
> DCL TEMP[0..1], LOCAL
> DCL ADDR[0]
> IMM[0] FLT32 {1., 0., 0., 0.}
> IMM[1] INT32 {1, 0, 0, 0}
>   0: MOV TEMP[0], IMM[0].xyyx
>   1: UARL ADDR[0].x, CONST[0][0].
>   2: USEQ TEMP[1].x, SV[ADDR[0].x]., IMM[1].

OK, this is a big problem. I'm guessing the GLSL code was something like

gl_SampleMaskIn[uniform]

What this got translated into was an indirect access into the *global
implicit array of system values*. We don't want that. glsl_to_tgsi
should just be dropping the indirect access entirely.

Cheers,

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gert Wollny]

Am Freitag, den 02.02.2018, 09:04 +0100 schrieb Roland Scheidegger:
> 
> 
> 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).
the TGSI for the relevant shader in the piglit looks like this: 

FRAG
DCL SV[0], SAMPLEMASK
DCL OUT[0], COLOR
DCL CONST[0][0]
DCL TEMP[0..1], LOCAL
DCL ADDR[0]
IMM[0] FLT32 {1., 0., 0., 0.}
IMM[1] INT32 {1, 0, 0, 0}
  0: MOV TEMP[0], IMM[0].xyyx
  1: UARL ADDR[0].x, CONST[0][0].
  2: USEQ TEMP[1].x, SV[ADDR[0].x]., IMM[1].
  3: UIF TEMP[1].
  4:   MOV TEMP[0].xy, IMM[0].yxyy
  5: ENDIF
  6: MOV OUT[0], TEMP[0]
  7: END

So it is kind of declared as an array of one element, and it is
accessed indirectly using the ADDR register, even though this must
currently be be zero on all supported hardware.

In tgsi_scan.c:246 the indirect flag is set based on 
src->Register.File if an indirect access is recorded, and given in the
TGSI it is an indirect access, the flag is, of course, set. 

This flag is then simply copied in r600_shader_from_tgsi and the only
time when TGSI_FILE_SYSTEM_VALUE becomes relevant is in r600/sb where
this patch would be applied. 

I haven't looked at how other drivers deal with this, but I think since
the way sb handles this is a regression compared to running without sb
it should be fixed there. 

Best, 
Gert 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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@arb_gpu_shader5@execution@samplemaskin-
>>> indirect for
>>> debug builds (it already passed in release).
>>>
>>> Signed-off-by: Gert Wollny 
>>> ---
>>>
>>>  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=DwICaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=FcYHgyjoyYUywC54bIyEgOTUo_kmJWgEGLt49fTYkO4=r1U2A6MGHIWSsd2qcriwKNcZYFcjPKBvUA97L3O58jw=
> .xhtml
> 
> [2]https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_mesa-2Ddev_2017-2DSeptember_16937=DwICaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=FcYHgyjoyYUywC54bIyEgOTUo_kmJWgEGLt49fTYkO4=xzMOlOUwz2OklnZ9QIEl_bezbIYRcvb0KqMfWuj3MMA=
> 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);
>>> }
>>> }
>>>
>>
>>

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[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@arb_gpu_shader5@execution@samplemaskin-
> > indirect for
> > debug builds (it already passed in release).
> > 
> > Signed-off-by: Gert Wollny 
> > ---
> > 
> >  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].

Best, 
Gert 

[1] 
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_SampleMask
.xhtml

[2]https://lists.freedesktop.org/archives/mesa-dev/2017-September/16937
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);
> > }
> > }
> > 
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[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@arb_gpu_shader5@execution@samplemaskin-indirect for
> debug builds (it already passed in release).
> 
> Signed-off-by: Gert Wollny 
> ---
> 
>  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.

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);
>   }
>   }
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev