[Mesa-dev] [PATCH 1/6] glsl: Optimize pow(x, 2) into x * x.

[Ian Romanick]

On 03/12/2014 01:29 AM, Erik Faye-Lund wrote:
> On Wed, Mar 12, 2014 at 1:32 AM, Eric Anholt <eric at anholt.net> wrote:
>> Erik Faye-Lund <kusmabite at gmail.com> writes:
>>
>>> On Wed, Mar 12, 2014 at 12:00 AM, Eric Anholt <eric at anholt.net> wrote:
>>>> Erik Faye-Lund <kusmabite at gmail.com> writes:
>>>>
>>>>> On Tue, Mar 11, 2014 at 7:27 PM, Eric Anholt <eric at anholt.net> wrote:
>>>>>> Erik Faye-Lund <kusmabite at gmail.com> writes:
>>>>>>
>>>>>>> On Tue, Mar 11, 2014 at 2:50 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>>>>>>>> On Mon, Mar 10, 2014 at 11:54 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>>>>>>>> Cuts two instructions out of SynMark's Gl32VSInstancing benchmark.
>>>>>>>>> ---
>>>>>>>>>  src/glsl/opt_algebraic.cpp | 8 ++++++++
>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
>>>>>>>>> index 5c49a78..8494bd9 100644
>>>>>>>>> --- a/src/glsl/opt_algebraic.cpp
>>>>>>>>> +++ b/src/glsl/opt_algebraic.cpp
>>>>>>>>> @@ -528,6 +528,14 @@ ir_algebraic_visitor::handle_expression(ir_expression *ir)
>>>>>>>>>        if (is_vec_two(op_const[0]))
>>>>>>>>>           return expr(ir_unop_exp2, ir->operands[1]);
>>>>>>>>>
>>>>>>>>> +      if (is_vec_two(op_const[1])) {
>>>>>>>>> +         ir_variable *x = new(ir) ir_variable(ir->operands[1]->type, "x",
>>>>>>>>> +                                              ir_var_temporary);
>>>>>>>>> +         base_ir->insert_before(x);
>>>>>>>>> +         base_ir->insert_before(assign(x, ir->operands[0]));
>>>>>>>>> +         return mul(x, x);
>>>>>>>>> +      }
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Is this safe? Since many GPUs implement pow(x, y) as exp2(log2(x) *
>>>>>>>> y), this will give different results for if y comes from a uniform vs
>>>>>>>> if it's a constant, no?
>>>>>>
>>>>>> Yes, but that wouldn't be covered by the "invariant" keyword.
>>>>>>
>>>>>>> To be a bit more clear: I don't think this is valid for expressions
>>>>>>> writing to variables marked as invariant (or expressions taking part
>>>>>>> in the calculations that leads up to invariant variable writes).
>>>>>>>
>>>>>>> I can't find anything allowing variance like this in the invariance
>>>>>>> section of the GLSL 3.30 specifications. In particular, the list
>>>>>>> following "To guarantee invariance of a particular output variable
>>>>>>> across two programs, the following must also be true" doesn't seem to
>>>>>>> require the values to be passed from the same source, only that the
>>>>>>> same values are passed. And in this case, the value 2.0 is usually
>>>>>>> exactly representable no matter what path it took there.
>>>>>>>
>>>>>>> Perhaps I'm being a bit too pedantic here, though.
>>>>>>
>>>>>> This file would do the same thing on the same expression tree in two
>>>>>> different programs, so "invariant" is fine (we've probably got other
>>>>>> problems with invariant, though).  The keyword you're probably thinking
>>>>>> of is "precise", which isn't in GLSL we implement yet.
>>>>>
>>>>> Are you saying that this only rewrites "x = pow(y, 2.0)" and not
>>>>> "const float e = 2.0; x = pow(y, e);"? If so, my point is moot,
>>>>> indeed. But if that's *not* the case, then I think we're in trouble
>>>>> still.
>>>>
>>>> The second would also get rewritten, because other passes will move the
>>>> 2.0 into the pow.
>>>>
>>>> I thought I understood your objection, but now I don't.  I think you'll
>>>> have to lay out the pair of shaders involving the invariant keyword that
>>>> you think that would be broken by this pass.
>>>
>>> My understanding is that
>>> ---8<---
>>> invariant varying float v;
>>> attribute float a;
>>> const float e = 2.0;
>>> void main()
>>> {
>>> v = pow(a, e);
>>> }
>>> ---8<---
>>> and
>>> ---8<---
>>> invariant varying float v;
>>> attribute float a;
>>> uniform float e;
>>> void main()
>>> {
>>> v = pow(a, e);
>>> }
>>> ---8<---
>>> ...should produce the exact same result, as long as the latter is
>>> passed 2.0 as the uniform e.
>>>
>>> Because v is marked as invariant, the expressions writing to v are the
>>> same, and the values passed in are the same.
>>>
>>> If we rewrite the first one to do "a * a", we get a different result
>>> on implementations that do "exp2(log2(a) * 2.0)" for the latter, due
>>> to floating-point normalization in the intermediate steps.
>>
>> I don't think that's what the spec authors intended from the keyword.  I
>> think what they intended was that if you had uniform float e in both
>> cases, but different code for setting *other* lvalues, that you'd still
>> get the same result in v.
> 
> I think that *might* be correct, but it doesn't seem to be what's
> actually defined. Invariance comes from ESSL, and FWIW, I was one of
> the spec authors in this case. But I don't remember the details down
> to this level, and the spec doesn't seem to clarify either.
> 
> However, since constant expressions are given some slack wrt how it's
> evaluated, I'm inclined to think that you're right about the spirit of
> the spec.
> 
> AFAIR, we introduced "invariant" to get rid of ftransform (since we'd
> already gotten rid of fixed-function state), so that multi-pass
> rendering algorthms could be guaranteed that all passes ended up
> covering the exact same fragments at the exact same depth coordinate.
> And in cases like that, the inputs would really be of the same kind
> all the time, I guess.

I believe the intention was to prevent inter-expression optimizations
from causing different shaders from producing different results.
Relative to this example, you can imagine:

attribute float a, b;
uniform c;

void main()
{
    a = pow(c, 3.0);
    b = pow(c, 5.0);
}

//----------

attribute float a, b;
uniform c;

void main()
{
    a = pow(c, 9.0);
    b = pow(c, 5.0);
}

In the first shader, we might flatten the calculate of a and b to
multiplies.  a would be c * c * c, and b would be a * c * c.  The second
shader might generate two POW instructions.  The invariant keyword would
(hypothetically) force the calculation of b to be the same in both.

If you had a similar situation involving the calculation of gl_Position,
you could get mis-aligned geometry.

Right now we do barely more than ignore the invariant keyword.  I don't
feel like this (or the other proposed) optimization is going to make
this worth.  I was chatting with Matt on IRC, and I feel like we should
revisit our poor treatment of invariance when we start implementing the
precise keyword with tessellation shaders.

FWIW... there are zero occurances of invariant in shader-db.  It's hard
to get excited about doing work to support a feature that doesn't see
much (if any!) use. :(

> So yeah, perhaps. But I wouldn't feel safe about optimizations like
> this without a clarification from Khronos.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev