On 25 November 2013 15:02, Eric Anholt <e...@anholt.net> wrote: > Paul Berry <stereotype...@gmail.com> writes: > > > In commit 065da16 (glsl: Convert lower_clip_distance_visitor to be an > > ir_rvalue_visitor), we failed to notice that since > > lower_clip_distance_visitor overrides visit_leave(ir_assignment *), > > ir_rvalue_visitor::visit_leave(ir_assignment *) wasn't getting called. > > As a result, clip distance dereferences appearing directly on the > > right hand side of an assignment (not in a subexpression) weren't > > getting properly lowered. This caused an ir_dereference_variable node > > to be left in the IR that referred to the old gl_ClipDistance > > variable. However, since the lowering pass replaces gl_ClipDistance > > with gl_ClipDistanceMESA, this turned into a dangling pointer when the > > IR got reparented. > > > > Prior to the introduction of geometry shaders, this bug was unlikely > > to arise, because (a) reading from gl_ClipDistance[i] in the fragment > > shader was rare, and (b) when it happened, it was likely that it would > > either appear in a subexpression, or be hoisted into a subexpression > > by tree grafting. > > > > However, in a geometry shader, we're likely to see a statement like > > this, which would trigger the bug: > > > > gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i]; > > These two are: > > Reviewed-by: Eric Anholt <e...@anholt.net> > > I didn't follow how gl_in[j].gl_ClipDistance[i] is a bare struct deref > of the thing needing to be lowered, since it reads to me like there's an > array and struct dereference first. But I assume that's just me not > understanding how gl_in[] works. >
Yeah, it's complicated by the fact that by the time we get to this code, lower_named_interface_blocks.cpp has munged gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i] into: gl_ClipDistance[i] = gl_ClipDistance[j][i] (note that the "gl_ClipDistance" appearing on the LHS refers to the GS output var, and the "gl_ClipDistance" appearing on the RHS refers to the GS input var). Also, I oversimplified a bit when I said "clip distance dereferences appearing directly on the right hand side of an assignment"; what I really meant was "either an array_deref of gl_ClipDistance out, or an array_deref of an array_deref of gl_ClipDistance in". But I figured that going into that level of detail would create more confusion than it solved. Anyway, thanks for hte review :)
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
