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

[Bryan Cain]

On 10/29/2012 06:22 AM, Andreas Boll wrote:
> 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.
>>>> actually is
>>> "-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
>>
>> actually isThose 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.

Yes, it is.  I just haven't gotten around to pushing it yet.  You can do
it if you'd like.

Bryan