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

[Andreas Boll]

2012/10/29 Bryan Cain <bryancain3 at gmail.com>:
> 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

Pushed, thanks.