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

[Alyssa Rosenzweig]

> 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 :)

> FWIW, since someone will probably comment, I like this organization
> better than the method other passes use of splitting things into
> multiple functions.

Hehe, thank you :)