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

[Kenneth Graunke]

On Friday, December 29, 2017 2:48:13 PM PST Eric Anholt wrote:
> 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)

Ah cool, I had forgotten that bit of the spec :)

[snip]
> 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);

Both of these comments sound good to me - I wouldn't bother with
anything more complicated.  NIR shouldn't really care about types
either...it's effectively just going to be bitcast.  You've got my:

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171229/6873b49d/attachment.sig>