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

[Paul Berry]

On 18 July 2012 15:12, Ian Romanick <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.)


> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120718/91fc1c5a/attachment-0001.html>