Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Expr issue #538

Closed
theChaosCoder opened this issue Mar 18, 2020 · 6 comments
Closed

Possible Expr issue #538

theChaosCoder opened this issue Mar 18, 2020 · 6 comments

Comments

@theChaosCoder
Copy link
Contributor

The script psharpen produces artifacts with R48 / R49. It works fine with R47.

The problem starts on line 64

expr = ('{x} {y} / 2 * 1 - abs {x0} < {s} 1 = {x} {y} 2 / = 0 {y} 2 / ? '
            '{x} {y} / 2 * 1 - abs 1 {s} - / ? {x} {y} / 2 * 1 - abs 1 {t} - '
            '* {t} + ? {x} {y} 2 / > 1 -1 ? * 1 + {y} * 2 / {scl} *').format(
                x=x, y=y, x0=x0, t=t, s=s, scl=scl)
    
nval = core.std.Expr([nval, nmax], [expr])

Look here https://gist.github.com/4re/2545a281e3f17ba6ef82#file-psharpen-py-L64

More info and image here: https://forum.doom9.org/showthread.php?t=180686

@theChaosCoder theChaosCoder changed the title Possible Expr error Possible Expr issue Mar 18, 2020
@sekrit-twc
Copy link
Contributor

Does it still happen with std.SetMaxCPU("none")?

@4re
Copy link

4re commented Mar 18, 2020

Nope.

@theChaosCoder
Copy link
Contributor Author

theChaosCoder commented Mar 18, 2020

With std.SetMaxCPU("none") it's ok.
std.SetMaxCPU("sse"), sse2, avx, avx2 are showing the same strange output.
Tested on a Ryzen 2600 Win64

@sekrit-twc
Copy link
Contributor

sekrit-twc commented Mar 18, 2020

The problem with this expression is that it depends on implementation-defined behavior. When the inputs x and y are both 0, then the expression evaluates to NaN, which propagates to the final value. When run through the interpreter, this NaN is converted to an integer by calling the standard library function lrintf, which returns 0 on Visual Studio. In JIT mode, the final value is converted by the x86 instruction cvtps2dq, which returns INT64_MIN (saturated to 255).

This expression is broken, because the C language specification does not require NaN to be converted to 0. On Linux, lrintf maps directly to cvtps2dq, which would produce the "wrong" result. More generally, std.Expr does not provide full IEEE-754 semantics, because it uses optimizations that are unsafe when NaN and INFINITY are present. Also, the implicit reliance on integer conversion to map NaN to 0 means that the expression does not work with floating-point clips.

The fix for this issue is to modify the expression so that it does not divide by zero. One approach would be to replace all instances of x y / with y 0 = TRAP x y / ?. Another approach would be to add a small value to y, i.e. x y 0 = y EPSILON + y ? /, or simply x y EPSILON + / if all possible values are positive.

@theChaosCoder
Copy link
Contributor Author

hmm ok but why does it work in R47? R47 must have also SSE etc. optimizations?

@myrsloik
Copy link
Member

I lot of code got rewritten in R48 to be faster. If it worked before it was a pure coincidence since I didn't bother with correct inf and nan handling there either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants