[Mesa-dev] [RFC PATCH 0/5] ARB_gpu_shader5 interpolateAt* GLSL plumbing

[Paul Berry]

On 10 November 2013 00:51, Chris Forbes <chrisf at ijw.co.nz> 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 <stereotype441 at gmail.com>

for now, and let me know if you'd like me to do a more thorough review.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131110/e548d553/attachment-0001.html>