[Mesa-dev] [PATCH] glsl_to_tgsi: set correct register type for array and structure elements

[Andreas Boll]

2012/10/24 Marek Olšák <maraeo at gmail.com>:
> On Tue, Oct 23, 2012 at 11:52 PM, Bryan Cain <bryancain3 at gmail.com> wrote:
>> On 10/23/2012 04:50 PM, Brian Paul wrote:
>>> On 10/23/2012 10:58 AM, Bryan Cain wrote:
>>>> This fixes an issue where glsl_to_tgsi_visior::get_opcode() would
>>>> emit the
>>>> wrong opcode because the register type was GLSL_TYPE_ARRAY/STRUCT
>>>> instead of
>>>> GLSL_TYPE_FLOAT/INT/UINT/BOOL, so the function would use the float
>>>> opcodes for
>>>> operations on integer or boolean values dereferenced from an array or
>>>> structure.  Assertions have been added to get_opcode() to prevent
>>>> this bug
>>>> from reappearing in the future.
>>>> ---
>>>>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp |   21
>>>> +++++++++++++++++++--
>>>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> index 9146f24..cefc568 100644
>>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> @@ -633,6 +633,11 @@ glsl_to_tgsi_visitor::get_opcode(ir_instruction
>>>> *ir, unsigned op,
>>>>   {
>>>>      int type = GLSL_TYPE_FLOAT;
>>>>
>>>> +   assert(src0.type != GLSL_TYPE_ARRAY);
>>>> +   assert(src0.type != GLSL_TYPE_STRUCT);
>>>> +   assert(src1.type != GLSL_TYPE_ARRAY);
>>>> +   assert(src1.type != GLSL_TYPE_STRUCT);
>>>> +
>>>>      if (src0.type == GLSL_TYPE_FLOAT || src1.type == GLSL_TYPE_FLOAT)
>>>>         type = GLSL_TYPE_FLOAT;
>>>>      else if (native_integers)
>>>> @@ -1074,8 +1079,12 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>>>                  assert(index == storage->index + (int)i);
>>>>               }
>>>>            } else {
>>>> -            st_src_reg src(PROGRAM_STATE_VAR, index,
>>>> -                  native_integers ? ir->type->base_type :
>>>> GLSL_TYPE_FLOAT);
>>>> +             /* We use GLSL_TYPE_FLOAT here regardless of the actual
>>>> type of
>>>> +              * the data being moved since MOV does not care about
>>>> the type of
>>>> +              * data it is moving, and we don't want to declare
>>>> registers with
>>>> +              * array or struct types.
>>>> +              */
>>>> +            st_src_reg src(PROGRAM_STATE_VAR, index, GLSL_TYPE_FLOAT);
>>>>               src.swizzle = slots[i].swizzle;
>>>>               emit(ir, TGSI_OPCODE_MOV, dst, src);
>>>>               /* even a float takes up a whole vec4 reg in a
>>>> struct/array. */
>>>> @@ -2042,6 +2051,9 @@
>>>> glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
>>>>      else
>>>>         src.swizzle = SWIZZLE_NOOP;
>>>>
>>>> +   /* Change the register type to the element type of the array. */
>>>> +   src.type = ir->type->base_type;
>>>> +
>>>>      this->result = src;
>>>>   }
>>>>
>>>> @@ -2067,6 +2079,7 @@
>>>> glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
>>>>         this->result.swizzle = SWIZZLE_NOOP;
>>>>
>>>>      this->result.index += offset;
>>>> +   this->result.type = ir->type->base_type;
>>>>   }
>>>>
>>>>   /**
>>>> @@ -2286,6 +2299,10 @@ glsl_to_tgsi_visitor::visit(ir_assignment *ir)
>>>>         inst->dead_mask = inst->dst.writemask;
>>>>      } else {
>>>>         for (i = 0; i<  type_size(ir->lhs->type); i++) {
>>>> +         if (ir->rhs->type->is_array())
>>>> +             r.type = ir->rhs->type->element_type()->base_type;
>>>> +         else if (ir->rhs->type->is_record())
>>>> +             r.type =
>>>> ir->rhs->type->fields.structure[i].type->base_type;
>>>>            emit(ir, TGSI_OPCODE_MOV, l, r);
>>>>            l.index++;
>>>>            r.index++;
>>>
>>> Thanks.  That seems to fix the test program that Jose posted to the
>>> piglit list (vs-all-equal-bool-array).
>>>
>>> Did you do a full piglit regression test?
>>>
>>> If so:
>>>
>>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>>
>>> But we should probably note this as a candidate for the stable branches.
>>>
>>> -Brian
>>
>> I did a piglit regression test with "-t shaders", which I think should
>> cover everything.  And yes, this should be a candidate for the stable
>> branches.  I just forgot to mention that in the commit message.
>
> "-t shaders" actually covers a very small subset of all GLSL tests.
> Most of them are in the spec group.
>
> Marek
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Tested-by: Andreas Boll <andreas.boll.dev at gmail.com>

I found no regression in piglit (quick-driver) on r600g (rv770),
llvmpipe and softpipe.

This patch fixes the following piglit tests:
{fs,vs}-bool-array on r600g and softpipe
{fs,vs}-int-array on r600g and softpipe
vs-all-equal-bool-array on llvmpipe

Those tests pass for me on mesa 8.0.4.
So this patch actually fixes regressions introduced somewhere on the road.

Nice work Bryan!