[Mesa-dev] [PATCH 2/4] mesa: Set UsesDFdy appropriately for assembly programs.

[Kenneth Graunke]

On 07/18/2012 04:09 PM, Paul Berry wrote:
> On 18 July 2012 15:12, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> 
>     On 07/18/2012 02:50 PM, Kenneth Graunke wrote:
> 
>         On 07/18/2012 12:16 PM, Paul Berry wrote:
> 
>             ---
>               src/mesa/program/arbprogparse.__c   |    1 +
>               src/mesa/program/program___parse.y  |    2 ++
>               src/mesa/program/program___parser.h |    1 +
>               3 files changed, 4 insertions(+), 0 deletions(-)
> 
>             diff --git a/src/mesa/program/__arbprogparse.c
>             b/src/mesa/program/__arbprogparse.c
>             index dffc8ab..72e51dd 100644
>             --- a/src/mesa/program/__arbprogparse.c
>             +++ b/src/mesa/program/__arbprogparse.c
>             @@ -120,6 +120,7 @@
>             _mesa_parse_arb_fragment___program(struct gl_context* ctx,
>             GLenum target,
>                  program->PixelCenterInteger =
>             state.option.__PixelCenterInteger;
> 
>                  program->UsesKill            = state.fragment.UsesKill;
>             +   program->UsesDFdy            = state.fragment.UsesDFdy;
> 
> 
>         This reminds me, could you do something similar to clean up the
>         kill_emitted flag in brw_fs_visitor?  It's kind of a hack, and I
>         seem to
>         recall that we wanted to do the kind of front-end plumbing you're
>         already doing here.
> 
> 
>         Not to mention there already seems to be a UsesKill flag...
> 
> 
> (Greps through the code a bit)  Oh, wow.  Yeah, there's a bunch of
> ugliness in there.  Things I would want to clean up:
> 
> 1. UsesKill is declared in core mesa (in gl_fragment_program), but it's
> only *sometimes* set by core Mesa (specifically, core mesa sets it for
> arb programs and GLSL programs that go through ir_to_mesa).  For GLSL
> programs that don't go through ir_to_mesa, it's set by the driver
> back-end (brw_link_shader() or glsl_to_tgsi_visitor).  Splitting the
> responsibility for setting the flag between core mesa and the back-end
> based on the type of program, that just seems like it's asking for
> future bugs.  This should be easy to fix by setting UsesKill in
> do_set_program_inouts(), which is already called from all the right places.
> 
> 2. brw_link_shader() sets UsesKill before doing optimizations.  That
> seems silly, since optimizations might be able to get rid of discards,
> in which case we shouldn't set the flag.  The same fix should address
> this problem too--if we set UsesKill in do_set_program_inouts(), then it
> will happen at the right time, after optimizations.
> 
> 3. Assuming we fix #2, kill_emitted should almost always be equal to
> UsesKill, so there's no point in having both.  (The only exception would
> be if front-end optimizations can't figure out how to eliminate
> discards, but back-end optimizations can.  That seems like a rare enough
> situation that it's not worth worrying about.)

Yeah...my thoughts exactly.  We should check for discards, dFdy, and
other features after GLSL IR level optimizations (ideally after they're
run from brw_shader.cpp), but not worry about BRW level optimizations
getting rid of them.

At some point, it might make sense to have a new visitor pass, rather
than piggy-backing on ir_set_program_inouts.  I agree that it seems like
overkill for one or two flags, but we might want to look for other
features in the future.  A specific pass that gathers "does the shader
use feature <X>?" flags would offer a nice place to add such checks in
the future.

>     I thought there were two problems with that.  First, we try to
>     optimize away the kill instructions, and it's hard to unset a flag.
>      Second, there are other things that cause kill_emitted to get set
>     (alpha test?).
> 
> 
> The first point should get taken care of by #2 above.
> 
> As to the second point, I believe you may be conflating the
> kill_enabled/UsesKill flags with the GEN{6,7}_WM_KILL_ENABLE flags.  The
> former set of flags is only set by discards.  The latter set of flags is
> derived from UsesKill by ORing with (ctx->Color.AlphaEnabled ||
> ctx->Multisample.SampleAlphaToCoverage), and this happens long after
> compilation has finished (see, for example, gen7_wm_state.c, around line
> 74).  The clean up I anticipate doing would only affect the
> kill_enabled/UsesKill flags, so GEN{6,7}_WM_KILL_ENABLE should still
> work properly.
> 
> I'll try putting together some clean-up patches for
> kill_enabled/UsesKill.  It looks like it shouldn't be too difficult.
> 
> Paul

Right, I'm just advocating getting rid of the "shader has a discard"
flag (which is kill_emitted).  Alpha testing and such still causes the
WM_KILL_ENABLE flag to be set in 3DSTATE_WM.