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