On 10 November 2013 00:51, Chris Forbes <[email protected]> wrote: > Here is the driver-independent part of ARB_gpu_shader5's > interpolateAtCentroid, interpolateAtOffset builtins. > > Before I go further with this approach, I'd like feedback on the following: > > 1) I've (ab)used ir_var_shader_in variable mode in function signatures to > enforce the strange restrictions interpolateAt* have. Is this crazy/awful? >
I'd rather not go this route if we can avoid it. Before commit 42a29d8, we had a similar situation where the same set of ir_variable_mode enums were used to reperesent shader ins/outs as function ins/outs, and it caused a lot of confusion and risk of mistakes in optimization and lowering passes. I'm glad we moved away from that and I'd like to avoid drifting back in that direction. I'd suggest instead adding a new field to the ir_variable class to represent this restriction (e.g. ir_variable::requires_shader_input). We would be set to true in the builtin_builder::_interpolateAt...() functions, and then in patch 1/5, instead of checking the restriction when formal->mode == ir_var_shader_in, we check it when formal->requires_shader_input is true. > > 2) I intend to implement interpolateAtSample() by: > > - Adding a new builtin uniform (perhaps "gl_SamplePositionsMESA"); which > will be an array of vec2, containing the full palette of current sample > positions. This could be formally exposed by another extension at a > later > point. > > - Compiling interpolateAtSample(x, sample_num) as if the shader author > wrote: > interpolateAtOffset(x, gl_SamplePositionsMESA[sample_num] - vec2(0.5)) > > Is this sensible? > > Does it match what other hardware will need to do? (it makes sense for > i965, > where the fragment shader payload otherwise does not have access to a full > palette of sample positions.) > I suspect that this won't make sense for a lot of hardware, because either: (a) the hardware may have a fast mechanism for doing interpolateAtSample() which is better than interpolateAtOffset(x, gl_SamplePositionsMESA[sample_num] - vec2(0.5)). In fact, even i965 has such a mechanism (the eval_sindex message), but it only works if the sample_num is uniform or constant, and it's easiest to use if it's constant. I suspect a lot of uses of interpolateAtSample are going to use a sample_num that's constant (after loop unrolling) so it seems worth using this fast mechanism when we can. (b) if the hardware supports variable sample locations, then this technique won't work at all because the values in gl_SamplePositionsMESA[sample_num] will need to vary from pixel to pixel. I'd be ok with an initial implementation of interpolateAtSample() based on interpolateAtOffset(), but I'd recommend doing it in the i965-specific fs_visitor (rather than in src/glsl) so that it doesn't get in the way of other hardware. Later when we want to add the fast mechanism that requires sample_num to be uniform or constant, it will be easy to add that to the fs_visitor implementation with a few "if" statements. Other than those two concerns I had a quick glance at the rest of the series and your approach seems reasonable to me. Consider it: Acked-by: Paul Berry <[email protected]> for now, and let me know if you'd like me to do a more thorough review.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
