Bug 103616

Summary: Increased difference from reference image in shaders
Product: Mesa Reporter: Ruslan Kabatsayev <b7.10110111>
Component: Drivers/DRI/i965Assignee: Matt Turner <mattst88>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: mattst88
Version: 17.2   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: patch

Description Ruslan Kabatsayev 2017-11-08 08:00:53 UTC
After commit 4009a9ead490ef1718e6fa83141aa086a43cd901 (found by git-bisect, double-checked by git-revert) I get two failures of the integration_test in precomputed_atmospheric_scattering demo [1]. The tests there compare reference CPU-generated image with the one generated with GLSL code, checking that PSNR is high enough. Before the above commit they all passed. Now the tests failing are:

47 is not less than 46.7812
[ FAIL ] ModelTest RadianceSeparateTextures
46 is not less than 45.6379
[ FAIL ] ModelTest RadianceCombineTextures

I tested on Xeon E3-1226 v3 built-in GPU, as well as on Core i7-4765T built-in GPU, with the exact same results.

I'm not sure if this is an expected decrease in precision, but it seems strange to me: from the commit message I get that the code transformation introduced should be equivalent. Apparently it's not, or there may be a bug somewhere.

[1]: https://github.com/ebruneton/precomputed_atmospheric_scattering
Comment 1 Ruslan Kabatsayev 2017-11-08 08:01:31 UTC
CCing author of the commit.
Comment 2 Matt Turner 2017-11-08 21:33:51 UTC
Thanks for the bug report.

I tried building precomputed_atmospheric_scattering, but I get:

% make integration_test                   
mkdir -p output/Release/atmosphere/reference
g++ -Wall -Wmain -pedantic -pedantic-errors -std=c++11 -I. -Iexternal -Iexternal/dimensional_types -Iexternal/glad/include -Iexternal/progress_bar -DNDEBUG -O3 -fexpensive-optimizations -c atmosphere/reference/functions.cc -o output/Release/atmosphere/reference/functions.o
In file included from ./atmosphere/reference/functions.h:42:0,
                 from atmosphere/reference/functions.cc:39:
./atmosphere/reference/definitions.h:53:24: fatal error: math/angle.h: No such file or directory
 #include "math/angle.h"
                        ^
compilation terminated.
make: *** [Makefile:113: output/Release/atmosphere/reference/functions.o] Error 1

Searching for other include files under math/, like "math/ternary_function.h" on Google I only see references to your project. Help?
Comment 3 Ruslan Kabatsayev 2017-11-09 05:30:46 UTC
(In reply to Matt Turner from comment #2)

> ./atmosphere/reference/definitions.h:53:24: fatal error: math/angle.h: No
> such file or directory
>  #include "math/angle.h"

Please do `git submodule update --init` (or clone the repo with --recursive option) to fetch the required files. After that, to build only integration test, you can say `make integration_test`. After building you'll have to wait several minutes while the CPU model generates data, but then after this run you can re-run the test as needed without delays.
Comment 4 Matt Turner 2017-11-20 19:35:06 UTC
Created attachment 135615 [details] [review]
patch

Thanks. I can build it and run the integration tests now.

(In reply to Ruslan Kabatsayev from comment #0)
> I tested on Xeon E3-1226 v3 built-in GPU, as well as on Core i7-4765T

Both of those are Haswells, FWIW.

Testing on a Skylake, all 13 tests fail regardless of the fix. I'll get my Haswell out and see what it looks like there.
Comment 5 Ruslan Kabatsayev 2017-11-20 19:57:46 UTC
The patch works great on my Core i7-4765T (all tests pass).
Comment 6 Matt Turner 2017-11-20 22:43:39 UTC
(In reply to Ruslan Kabatsayev from comment #5)
> The patch works great on my Core i7-4765T (all tests pass).

Great! Thank you. I've sent that patch to the mailing list for review along with some new unit test cases for the case that was broken.
Comment 7 Matt Turner 2017-11-21 22:40:12 UTC
Thanks again for the bug report and bisection!

Fixed by

commit a05af1f7b8f82a38513bba31f9573cd62d82f18d
Author: Matt Turner <mattst88@gmail.com>
Date:   Mon Nov 20 14:24:57 2017 -0800

    i965/fs: Handle negating immediates on MADs when propagating saturates
    
    MADs don't take immediate sources, but we allow them in the IR since it
    simplifies a lot of things. I neglected to consider that case.
    
    Fixes: 4009a9ead490 ("i965/fs: Allow saturate propagation to propagate
                          negations into MADs.")
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103616
    Reported-and-Tested-by: Ruslan Kabatsayev <b7.10110111@gmail.com>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

and tested by

commit beaea7abfa9f25460284f739fa50050a889ce6cd
Author: Matt Turner <mattst88@gmail.com>
Date:   Mon Nov 20 14:21:43 2017 -0800

    i965/fs: Check ADD/MAD with immediates in satprop unit test
    
    The gen had to be changed from 4 to 6 so that we could test MAD, which
    is new on Gen6.
    
    mad_imm_float_neg_mov_sat tests the case fixed by the previous commit.
    
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.