[Mesa-dev] [PATCH 9/9] i965: Use NIR by default for Fragment shaders on SNB+

[Ilia Mirkin]

On Wed, Mar 18, 2015 at 11:27 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Mar 18, 2015 at 7:22 AM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> On Wed, Mar 18, 2015 at 1:56 AM, Matt Turner <mattst88 at gmail.com> wrote:
>>> Thanks. Looked through stats and at some of the regressions.
>>>
>>> Some of the areas I noticed we were doing worse:
>>>
>>> We generate two CMPs for discard_if; only one without NIR. I think you
>>> had an idea about solving this.
>>>
>>> SEL peephole interference -- we knew about this in general, but I
>>> found an interesting failure mode:
>>>
>>> NIR generates
>>>
>>> (+f0) if(8)     JIP: 4          UIP: 5
>>> else(8)         JIP: 3
>>> mov(8)          g124<1>F        -g124<8,8,1>F
>>> endif(8)        JIP: 6
>>>
>>> We should of course improve dead-control-flow-elimination to remove
>>> the else and set the if's predicate_inverse, but non-NIR generates
>>>
>>> (+f0) sel(8)    g124<1>F        g25<8,8,1>F     -g25<8,8,1>F
>>>
>>> which is probably better even than a predicated MOV since it's not a
>>> partial write and subject to optimization restrictions. The assembly
>>> matches the NIR exactly, so I'm assuming NIR is removing a
>>> self-assignment in the if-then block, preventing it from turning it
>>> into a conditional select.
>>
>> I think this is because the sel peephole (along with all the other
>> optimizations) in NIR only works before we lower to source mods, where
>> this code probably looked something like:
>>
>> ssa_1 = ...;
>> if ... {
>> } else {
>>     ssa_2 = fneg ssa_1;
>> }
>> ssa_3 = phi ssa_1, ssa_2;
>
> More specifically, we probably got
>
> b = foo ? a : -a
>
> from the GLSL source which NIR turns into
>
> if (foo) {
>    ssa_1 = a;
> } else {
>    ssa_2 = -a;
> }
> ssa_3 = phi(ssa_1, ssa_2)
>
> which gets copy-propagated to what Connor mentioned above.
>
>> So we can probably fix it just by adding fneg/ineg to
>> block_check_for_allowed_instrs().
>
> That was my first thought but we're starting to get into the world of
> backend-specific knowledge there.  Maybe this is where we just need to
> pass in Eric's backend info struct and predicate doing so on whether
> or not neg is free?  But I guess the only compiler we care about at
> the moment where that isn't true is vc4 which can't do control-flow
> right now anyway.  Thoughts?
>
> We may also want to simply make the backend's peephole smarter.

In case you're interested, the way nouveau/codegen solves this issue
is with target-specific callbacks that tell it what various
instructions can and cannot do, e.g.:

http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp#n302
http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp#n393

(and you can find equivalent functions for the nv50 target). Of course
nv50 and nvc0 are *fairly* similar to one another, but they do have a
lot of differences, including how they handle predication.

Cheers,

  -ilia