<div dir="ltr">On 10 November 2013 00:51, Chris Forbes <span dir="ltr"><<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Here is the driver-independent part of ARB_gpu_shader5's<br>
interpolateAtCentroid, interpolateAtOffset builtins.<br>
<br>
Before I go further with this approach, I'd like feedback on the following:<br>
<br>
1) I've (ab)used ir_var_shader_in variable mode in function signatures to<br>
enforce the strange restrictions interpolateAt* have. Is this crazy/awful?<br></blockquote><div><br></div>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.<br>
<div>
<br></div><div>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
2) I intend to implement interpolateAtSample() by:<br>
<br>
- Adding a new builtin uniform (perhaps "gl_SamplePositionsMESA"); which<br>
will be an array of vec2, containing the full palette of current sample<br>
positions. This could be formally exposed by another extension at a later<br>
point.<br>
<br>
- Compiling interpolateAtSample(x, sample_num) as if the shader author wrote:<br>
interpolateAtOffset(x, gl_SamplePositionsMESA[sample_num] - vec2(0.5))<br>
<br>
Is this sensible? <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Does it match what other hardware will need to do? (it makes sense for i965,<br>
where the fragment shader payload otherwise does not have access to a full<br>
palette of sample positions.)<br></blockquote><div><br></div><div>I suspect that this won't make sense for a lot of hardware, because either:<br><br>(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.<br>
<br>(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.<br><br></div><div>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.<br>
<br><br></div><div>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:<br><br></div><div>Acked-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
<br>for now, and let me know if you'd like me to do a more thorough review.<br></div></div></div></div>