[Mesa-dev] [PATCH 4/7] nir: Add nir_lower_blend pass

[Eric Anholt]

Alyssa Rosenzweig <alyssa at rosenzweig.io> writes:

>> Logic ops seem... challenging to emulate in the shader.  That shader
>> would need the destination colors in the framebuffer storage format, and
>> I'm not sure that's always possible (maybe?). 
>
> Alright, that's good to know.
>
> I will note that in Midgard, the native hardware ops are to load/store
> the actual framebuffer storage format. As seen later in the series, I
> lower load/store_output to a series of ops converting to/from float and
> the actual format. It's an open-question whether we want actual
> nir_intrinsics for this so the lowering happens in common NIR code and
> then we get fun logic ops and such.
>
>> It might also be fun to add support for GL_AMD_blend_minmax_factor and
>> GL_SGIX_blend_alpha_minmax.  Looking at this lowering pass, it seems
>> like most of the work would be adding tests.
>
> Probably!
>
>> Having all of the lowering related to blending in one place seems like a
>> good idea.
>
> Probably, yes. Also since GLSL IR is, well.... ;)
>
>> > +/* These structs encapsulates the blend state such that it can be lowered
>>                     encapsulate
>> > + * cleanly */
>> 
>> */ on its own line.  There's at least one more instance of this below.
>
> Is there, uh, a thing I can stick in my vimrc to get this right?
>
>> Alas, case should be at the same indentation level as switch.
>
> Good to know, thank you.
>
>> Since factor is an enum, I think it's better to not have the default
>> case.  If there's no default case, the compiler will issue a warning if
>> a new enum is added but not handled.
>> 
>> Either way, the break is definitely not necessary.
>
> +1
>
>> { goes on it's own line.
>
> --Wait, that's a rule I actually follow, how'd I mess it up here? :P
>
>> Why?  Without a vectorizer (or even with a vectorizer), it seems like
>> this will generate much worse code.  I guess it's only a few
>> instructions once per shader, so it may not matter... but it's a little
>> surprising especially after going to extra effort to get the blend color
>> as a vector.
>
> Honestly? Since that's how vc4 (scalar) did it and for v1 of this series I was
> more eager to get something passing tests than particularly good. In my
> original implementation (before joining upstream), I did it like this
> and then used nir_opt_vectorize to get it back to something reasonable.
> That's one strategy still. The other option is to try to generate vector
> out of the gate, but that's hard to get right when RGB/alpha can be
> separate (but don't have to be), etc. The logic needed would approach
> just, well, having ALU vectorization... might as well merge the
> vectorize pass at that point...
>
> Suggestions welcome :)

It does seem like we'll want vector some day for panfrost, but make
tests work first and fix perf later (knowing that it's fixable) is fine
with me.
-------------- 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/20190509/79fbf651/attachment.sig>