[Mesa-dev] [PATCH 1/3] nir: Add a lowering pass for gl_FragColor to glFragData[] writes.

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> [ Unknown signature status ]
> On Thursday, December 28, 2017 5:56:18 PM PST Eric Anholt wrote:
>> For VC5, the shader needs to have the appropriate base type for the
>> variable in the render target write, and gallium's
>> FS_COLOR0_WRITES_ALL_CBUFS (used for glClearBufferiv) doesn't give you
>> that information.  This pass lets the backend decide what types to explode
>> the gl_FragColor write out to.
>> 
>> This would also be a prerequisite of moving some of VC5's render target
>> format packing into NIR as well.
>
> This type confusion seems a bit unfortunate.  Both gl_FragColor and
> gl_FragData[i] are always float vec4s.  But, we've used (abused?) it in
> the past for integer clear/blit paths, since it's the only way to splat
> out to all render targets.
>
> I suppose this must be an internally generated shader?  Or, do you also
> need this for people writing to gl_FragColor but binding integer render
> targets?  (If so, what about people declaring 'out vec4 foo' but binding
> an integer render target?)

Yeah, it's gallium's internal glClearBufferiv() shader, translated using
TGSI-to-NIR.  I had initially written this pass without the types
argument, but found that adding it got me a whole bunch more tests
working.  I suppose we could move this into TTN and delay TTN until draw
time, to make sure that we always maintain proper types in the NIR, but
this was pretty easy.  How about adding this to the top comment:

 * The pass's types argument lets the caller choose between vec4, uvec4, and
 * ivec4 per render target.  This is necessary for TGSI-to-NIR output, which
 * emits vec4 gl_FragColor writes even for integer render targets (because
 * TGSI doesn't know the types).

If you use "out vec4 foo" but bind integer, you get "If the values
written by the fragment shader do not match the format(s) of the
corresponding color buffer(s), the result is undefined." (GL 4.3 spec)

>> +/**
>> + * Lowers gl_FragColor to a per-render-target store.
>> + *
>> + * GLSL's gl_FragColor writes implicitly broadcast their store to every active
>> + * render target.  This can be used by driver backends to implement
>> + * gl_FragColor in the same way as other multiple-render-target shaders, and
>> + * is particularly useful if the driver needs to do other per-render-target
>> + * lowering in NIR.
>> + *
>> + * Run before nir_lower_io.
>> + */
>> +
>> +typedef struct {
>
> Let's avoid typedefs here - this is just a struct containing the state
> of the pass.  We do use typedefs in NIR, but mostly for core concepts.
> This isn't anything that special or fundamental (and most passes just
> use a plain struct).

Agreed.  I think I was just following core NIR.

>> +   nir_shader *shader;
>> +   nir_builder b;
>> +
>> +   nir_variable *var; /* gl_FragColor */
>> +
>> +   int num_rt_vars;
>> +   nir_variable *rt_var[32]; /* gl_FragDataN */
>> +} lower_gl_fragcolor_state;
>> +
>> +static void
>> +lower_gl_fragcolor(lower_gl_fragcolor_state *state, nir_intrinsic_instr *intr)
>> +{
>> +   nir_builder *b = &state->b;
>> +
>> +   assert(intr->dest.is_ssa);
>> +
>> +   b->cursor = nir_before_instr(&intr->instr);
>> +
>> +   /* Generate a gl_FragDataN write per render target. */
>> +   nir_ssa_def *color = nir_ssa_for_src(b, intr->src[0], 4);
>> +   for (int i = 0; i < state->num_rt_vars; i++) {
>> +      nir_store_var(b, state->rt_var[i], color, 0xf);
>> +   }
>> +
>> +   /* Remove the gl_FragColor write. */
>> +   nir_instr_remove(&intr->instr);
>> +}
>> +
>> +static bool
>> +lower_gl_fragcolor_block(lower_gl_fragcolor_state *state, nir_block *block)
>> +{
>> +   nir_foreach_instr_safe(instr, block) {
>> +      if (instr->type == nir_instr_type_intrinsic) {
>> +         nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
>> +         if (intr->intrinsic == nir_intrinsic_store_var) {
>> +            nir_deref_var *dvar = intr->variables[0];
>> +            nir_variable *var = dvar->var;
>> +
>> +            if (var == state->var) {
>> +               /* gl_FragColor should not have array/struct derefs: */
>> +               assert(dvar->deref.child == NULL);
>> +               lower_gl_fragcolor(state, intr);
>> +            }
>> +         }
>> +      }
>> +   }
>> +
>> +   return true;
>> +}
>> +
>> +bool
>> +nir_lower_gl_fragcolor(nir_shader *shader, const uint32_t rt_mask,
>> +                      const struct glsl_type **types)
>> +{
>> +   lower_gl_fragcolor_state state = {
>> +      .shader = shader,
>> +   };
>> +
>> +   assert(shader->info.stage == MESA_SHADER_FRAGMENT);
>> +
>> +   nir_foreach_variable(var, &shader->outputs) {
>> +      if (var->data.location == FRAG_RESULT_COLOR) {
>> +         state.var = var;
>> +         break;
>> +      }
>> +   }
>> +
>> +   if (!state.var)
>> +      return false;
>> +
>> +   for (int i = 0; i < ARRAY_SIZE(state.rt_var); i++) {
>> +      if (!(rt_mask & (1 << i)))
>> +         continue;
>> +
>> +      char name[16];
>> +      snprintf(name, sizeof(name), "gl_FragData%d", i);
>> +
>
> Earlier, you generated store_vars with a writemask of 0xf, which assumes
> that these rt_var variables are gvec4's.  That's only reasonable if the
> underlying types are a gvec4.  Since you allow the caller to pass in an
> array of -any- glsl_type, it might be nice to enforce that here:
>
>     assert(glsl_get_components(types[i]) == 4);
>
> That should guard against the caller passing in bogus types with too few
> components for gl_FragColor lowering, or arrays/structures/craziness.

How about:

      /* We only emit 4-component stores above.  If you need to lower to fewer
       * components based on render target format, you probably need to do
       * that in a separate pass anyway to cover the non-gl_FragColor case.
       */
      assert(glsl_get_components(types[i]) == 4);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171229/4ce4f89b/attachment.sig>