2012/10/29 Bryan Cain <[email protected]>: > On 10/29/2012 06:22 AM, Andreas Boll wrote: >> 2012/10/24 Andreas Boll <[email protected]>: >>> 2012/10/24 Marek Olšák <[email protected]>: >>>> On Tue, Oct 23, 2012 at 11:52 PM, Bryan Cain <[email protected]> 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 <[email protected]> >>>>>> >>>>>> 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 >>>> [email protected] >>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> Tested-by: Andreas Boll <[email protected]> >>> >>> 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. _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
