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

[Andreas Boll]

2012/10/24 Andreas Boll <andreas.boll.dev at gmail.com>:
> 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!

Sorry I've pushed this commit accidentally.
I've reverted it for now.
But I think this patch is ready to land on master.
(with the stable note, the reviewed-by and tested-by)

Andreas.