[RFC] nir/algebraic: Simplify max(abs(a), 0.0) -> abs(a)

Submitted by Alyssa Rosenzweig on May 24, 2019, 2:47 a.m.

Details

Message ID 20190524024735.4348-1-alyssa@rosenzweig.io
State New
Headers show
Series "nir/algebraic: Simplify max(abs(a), 0.0) -> abs(a)" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alyssa Rosenzweig May 24, 2019, 2:47 a.m.
I noticed this pattern in glmark's jellyfish scene.

Assuming this is correct (it should be...?), could someone do a
shader-db run? Thank you!

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: Ian Romanick <ian.d.romanick@intel.com>
---
 src/compiler/nir/nir_opt_algebraic.py | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py
index 89d07aa1261..abd0b6591ce 100644
--- a/src/compiler/nir/nir_opt_algebraic.py
+++ b/src/compiler/nir/nir_opt_algebraic.py
@@ -377,6 +377,7 @@  optimizations = [
    (('imax', a, a), a),
    (('umin', a, a), a),
    (('umax', a, a), a),
+   (('fmax', ('fabs', a), 0.0), ('fabs', a)),
    (('fmax', ('fmax', a, b), b), ('fmax', a, b)),
    (('umax', ('umax', a, b), b), ('umax', a, b)),
    (('imax', ('imax', a, b), b), ('imax', a, b)),

Comments

How does max(NaN, 0) work? IIRC there's some provision that this
becomes 0, while abs(NaN) = NaN.

On Thu, May 23, 2019 at 10:47 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> I noticed this pattern in glmark's jellyfish scene.
>
> Assuming this is correct (it should be...?), could someone do a
> shader-db run? Thank you!
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Ian Romanick <ian.d.romanick@intel.com>
> ---
>  src/compiler/nir/nir_opt_algebraic.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py
> index 89d07aa1261..abd0b6591ce 100644
> --- a/src/compiler/nir/nir_opt_algebraic.py
> +++ b/src/compiler/nir/nir_opt_algebraic.py
> @@ -377,6 +377,7 @@ optimizations = [
>     (('imax', a, a), a),
>     (('umin', a, a), a),
>     (('umax', a, a), a),
> +   (('fmax', ('fabs', a), 0.0), ('fabs', a)),
>     (('fmax', ('fmax', a, b), b), ('fmax', a, b)),
>     (('umax', ('umax', a, b), b), ('umax', a, b)),
>     (('imax', ('imax', a, b), b), ('imax', a, b)),
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, May 23, 2019 at 10:54:38PM -0400, Ilia Mirkin wrote:
> How does max(NaN, 0) work? IIRC there's some provision that this
> becomes 0, while abs(NaN) = NaN.

Yes max(NaN, 0) should return 0.
At least it's what we do with i965.
See https://gitlab.freedesktop.org/mesa/mesa/blob/a42163cbbc1abe02b7db4ade74b569f455942d1a/src/compiler/glsl/float64.glsl#L1739
> 
> On Thu, May 23, 2019 at 10:47 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> >
> > I noticed this pattern in glmark's jellyfish scene.
> >
> > Assuming this is correct (it should be...?), could someone do a
> > shader-db run? Thank you!
> >
> > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> > Cc: Ian Romanick <ian.d.romanick@intel.com>
> > ---
> >  src/compiler/nir/nir_opt_algebraic.py | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py
> > index 89d07aa1261..abd0b6591ce 100644
> > --- a/src/compiler/nir/nir_opt_algebraic.py
> > +++ b/src/compiler/nir/nir_opt_algebraic.py
> > @@ -377,6 +377,7 @@ optimizations = [
> >     (('imax', a, a), a),
> >     (('umin', a, a), a),
> >     (('umax', a, a), a),
> > +   (('fmax', ('fabs', a), 0.0), ('fabs', a)),
> >     (('fmax', ('fmax', a, b), b), ('fmax', a, b)),
> >     (('umax', ('umax', a, b), b), ('umax', a, b)),
> >     (('imax', ('imax', a, b), b), ('imax', a, b)),
> > --
> > 2.20.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 5/23/19 7:54 PM, Ilia Mirkin wrote:
> How does max(NaN, 0) work? IIRC there's some provision that this
> becomes 0, while abs(NaN) = NaN.

That is correct.  There are a couple other algebraic patterns that have
the same potential problem (e.g., the min(max()) -> sat() patterns), and
we just make those as imprecise with ~.  I /think/ that should be
adequate here too.

> On Thu, May 23, 2019 at 10:47 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>>
>> I noticed this pattern in glmark's jellyfish scene.
>>
>> Assuming this is correct (it should be...?), could someone do a
>> shader-db run? Thank you!
>>
>> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
>> Cc: Ian Romanick <ian.d.romanick@intel.com>
>> ---
>>  src/compiler/nir/nir_opt_algebraic.py | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py
>> index 89d07aa1261..abd0b6591ce 100644
>> --- a/src/compiler/nir/nir_opt_algebraic.py
>> +++ b/src/compiler/nir/nir_opt_algebraic.py
>> @@ -377,6 +377,7 @@ optimizations = [
>>     (('imax', a, a), a),
>>     (('umin', a, a), a),
>>     (('umax', a, a), a),
>> +   (('fmax', ('fabs', a), 0.0), ('fabs', a)),
>>     (('fmax', ('fmax', a, b), b), ('fmax', a, b)),
>>     (('umax', ('umax', a, b), b), ('umax', a, b)),
>>     (('imax', ('imax', a, b), b), ('imax', a, b)),
>> --
>> 2.20.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> I /think/ that should be adequate here too.

Do inexact values not need to handle NaNs strictly, then? I'm not sure
what this corresponds to in the various GLs/CLs/Vulkan specs, hence
labeling this RFC.
On Fri, May 24, 2019 at 2:46 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> > I /think/ that should be adequate here too.
>
> Do inexact values not need to handle NaNs strictly, then? I'm not sure
> what this corresponds to in the various GLs/CLs/Vulkan specs, hence
> labeling this RFC.

I don't know about Vulkan, but GL has a very childish approach to
NaN's -- you can pretty much do whatever and be in-spec.

Applications fall into two categories -- ones that expect NaN's and
carefully deal with them (maybe DX10, definitely DX11+), and ones that
are from a time before NaN on GPUs existed (DX9 and earlier).

I'm generally a lot less concerned about inexact opts that remove
NaN's (e.g. a - a -> 0, but Inf - Inf = NaN, or a * 0 -> 0, but Inf *
0 = NaN) than ones that add NaN's such as this one. I don't have a
specific example, but I can well imagine an application that would
expect this to have cleared out any NaN's, and you'll suddenly remove
their op.

But I also see the benefit of doing it since 99.99999% of the time
it's perfectly safe :)

  -ilia