[Mesa-dev] Mesa (master): st/mesa: fix incorrect size of UBO declarations

[Brian Paul]

On 07/02/2014 06:36 AM, Brian Paul wrote:
> On 07/01/2014 08:43 PM, Michel Dänzer wrote:
>> On 02.07.2014 00:43, Brian Paul wrote:
>>> Module: Mesa
>>> Branch: master
>>> Commit: f4b0ab7afd83c811329211eae8167c9bf238870c
>>> URL:
>>> https://urldefense.proofpoint.com/v1/url?u=http://cgit.freedesktop.org/mesa/mesa/commit/?id%3Df4b0ab7afd83c811329211eae8167c9bf238870c&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=lGQMzzTgII0I7jefp2FHq7WtZ%2BTLs8wadB%2BiIj9xpBY%3D%0A&m=X5dPXUSLXb%2B60RW5%2FHCgcIkPXz083MgfZAbiRJFVOxc%3D%0A&s=c9766b9e3797ccad30888c534fae5891f9a7313418a228c81c0528230412cf1c
>>>
>>>
>>> Author: Brian Paul <brianp at vmware.com>
>>> Date:   Tue Jul  1 08:17:09 2014 -0600
>>>
>>> st/mesa: fix incorrect size of UBO declarations
>>>
>>> UniformBufferSize is in bytes so we need to divide by 16 to get the
>>> number of constant buffer slots.  Also, the ureg_DECL_constant2D()
>>> function takes first..last parameters so we need to subtract one
>>> for the last value.
>>
>> This change broke the GLSL uniform_buffer fs/vs/gs-struct-pad piglit
>> tests with radeonsi:
>>
>> ../../../../src/gallium/auxiliary/gallivm/lp_bld_tgsi.c:306:lp_build_emit_fetch:
>> Assertion `reg->Register.Index <=
>> bld_base->info->file_max[reg->Register.File]' failed.
>>
>> AFAICT reg->Register.Index is 2, and the new code calls
>> ureg_DECL_constant2D() with last=1. This is the TGSI code:
>>
>> FRAG
>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>> DCL OUT[0], COLOR
>> DCL CONST[1][0..1]
>> DCL TEMP[0], LOCAL
>> IMM[0] UINT32 {0, 16, 32, 0}
>>    0: MOV TEMP[0].x, CONST[1][0].xxxx
>>    1: MOV TEMP[0].y, CONST[1][1].xxxx
>>    2: MOV TEMP[0].z, CONST[1][2].xxxx
>>    3: MOV TEMP[0].w, CONST[1][2].xxxx
>>    4: MOV OUT[0], TEMP[0]
>>    5: END
>>
>> which results from this GLSL code:
>>
>> #version 140
>>
>> struct S1 {
>>          float r;
>> };
>>
>> struct S2 {
>>          float g;
>>          float b;
>>          float a;
>> };
>>
>> struct S {
>>         S1 s1;
>>         S2 s2;
>> };
>>
>> uniform ubo1 {
>>          S s;
>> };
>>
>> void main()
>> {
>>          gl_FragColor = vec4(s.s1.r, s.s2.g, s.s2.b, s.s2.a);
>> }
>>
>>
>> Something doesn't seem to add up, but I'm not sure where exactly the
>> problem is or how to fix it.
>
>
> I think there's at least a one GLSL compiler bug exposed here.
>
>  From the code fragments above, the z and w components of TEMP[0] are
> getting the same value when they should not.
>
> I reverted Mesa to before my changes and modified the piglit test to use
> different values for the blue/alpha channels and it fails (nvidia
> passes).  We were just getting lucky before my changes.
>
> I suspect the code which is laying out the floats in the UBO is doing
> something wrong, and the size of the UBO is miscalculated as a result.
>
> I'll dig a bit more but I think someone more familiar with the compiler
> will need to help.

The initial IR looks OK:


(loop ...

         (assign  (xyzw) (var_ref gl_Position)  (array_ref (var_ref 
vertex_to_gs) (var_ref i) ) )
         (declare (temporary ) vec4 vec_ctor)
         (assign  (x) (var_ref vec_ctor)  (record_ref (record_ref 
(var_ref s)  s1)  r) )
         (assign  (y) (var_ref vec_ctor)  (record_ref (record_ref 
(var_ref s)  s2)  g) )
         (assign  (z) (var_ref vec_ctor)  (record_ref (record_ref 
(var_ref s)  s2)  b) )
         (assign  (w) (var_ref vec_ctor)  (record_ref (record_ref 
(var_ref s)  s2)  a) )
         (assign  (xyzw) (var_ref v)  (var_ref vec_ctor) )
         (call EmitVertex  ())

... endloop)


But later, after linking, the Z and W components of the vector 
constructor seem to get merged.  Note: there's only 3 ubo_load 
instructions now instead of four:

...
       (assign  (xyzw) (var_ref gl_Position)  (array_ref (var_ref 
vertex_to_gs) (constant int (0)) ) )
       (declare (temporary ) vec4 vec_ctor)
       (declare () float cse)
       (assign  (x) (var_ref cse)  (expression float ubo_load (constant 
uint (0)) (constant uint (0)) ) )
       (assign  (x) (var_ref vec_ctor)  (var_ref cse) )
       (declare () float cse at 3)
       (assign  (x) (var_ref cse at 3)  (expression float ubo_load 
(constant uint (0)) (constant uint (16)) ) )
       (assign  (y) (var_ref vec_ctor)  (var_ref cse at 3) )
       (declare () float cse at 4)
       (assign  (x) (var_ref cse at 4)  (expression float ubo_load 
(constant uint (0)) (constant uint (32)) ) )
       (assign  (z) (var_ref vec_ctor)  (var_ref cse at 4) )
       (assign  (w) (var_ref vec_ctor)  (var_ref cse at 4) )
       (assign  (xyzw) (var_ref v)  (var_ref vec_ctor) )
       (assign  (xyzw) (var_ref v at 5)  (var_ref v) )
       (assign  (xyzw) (var_ref gl_Position at 6)  (var_ref gl_Position) )
       (emit-vertex (constant int (0)) )

I'm not sure I can debug this much further right now.

-Brian