Found this regression while testing 10.6.6 on a bsw Chromebook (cyan) with additional Chrome OS patches. The same test passes on 10.6.6 on snd, byt, hsw, bdw, ivb and skl Chrome OS devices. I reran the test 5 times on bsw and it consistently failed. Failure message: "Number of accurate bits assumed = 32", "iter 0, test 0: in0 = -1056263790, in1 = -1493720033, ref out = 971704494 / 0x39eb08ae", "Comparison failed (at 0, 0): got 1119553710 / 0x42bb08ae" When the test passes, the output has the 'accurate bits' line, followed by 128 lines similar to this: "iter 0, test 0: in0 = -1056263790, in1 = -1493720033, ref out = 971704494 / 0x39eb08ae", "iter 0, test 1: in0 = 1938634019, in1 = -1133464026, ref out = 2114766130 / 0x7e0cc532", "iter 0, test 2: in0 = 677512731, in1 = 867076180, ref out = -624797476 / 0xdac258dc",
Mark Janes is bisecting.
I bisected to this to 1e4e17fbd9296cc5064aabdb351a894d10190cb6 i965/fs: Lower integer multiplication after optimizations. I noticed that the test did not fail reliably, so I will repeat the bisect. Matt, does the commit seems like a plausible source of this failure?
Yes, that seems very plausible.
Created attachment 118317 [details] [review] patch The relevant instructions generated are mul(8) g124<1>D g6<8,8,1>D g10<8,8,1>UW { align1 1Q }; mul(8) g7<1>D g6<8,8,1>D g10<8,8,1>UW { align1 1Q }; ... add(8) g124.1<2>UW g124.1<16,8,2>UW g7<16,8,2>UW { align1 1Q }; I could imagine that the D * UW multiplies don't work as expected (maybe we should do D * W for signed multiplication?). Attached is a patch that gives that a try.
My second bisect confirmed the commit that regressed this test. I am testing Matt's patch. BTW, that patch doesn't apply cleanly to 10.6.
The patch provided breaks a bunch of tests, and doesn't fix the one listed in the bug.
(In reply to Matt Turner from comment #4) > Created attachment 118317 [details] [review] [review] > patch > > The relevant instructions generated are > > mul(8) g124<1>D g6<8,8,1>D g10<8,8,1>UW { align1 1Q > }; > mul(8) g7<1>D g6<8,8,1>D g10<8,8,1>UW { align1 1Q We're missing a .1 subreg offset on one of these. > }; > ... > add(8) g124.1<2>UW g124.1<16,8,2>UW g7<16,8,2>UW { align1 1Q > }; > > I could imagine that the D * UW multiplies don't work as expected (maybe we > should do D * W for signed multiplication?). Attached is a patch that gives > that a try.
If you enable the imul lowering on BDW, it fails there too.
g10 is from from ATTR file and we dont respect the subreg offset and stride for those.
Created attachment 118418 [details] [review] Fix We need to carry the ATTR stride and subreg information over to the hw reg when we assign hw regs to attributes.
Pushed to master: commit 2ea16966ae66d4dd5c5dcb996d7996d9c734bbee Author: Kristian Høgsberg Kristensen <krh@bitplanet.net> Date: Wed Sep 23 16:57:47 2015 -0700 i965: Respect stride and subreg_offset for ATTR registers When we assign hw regs to attributes, we don't incorporate the stride and subreg_offset from the fs_reg. It's rarely used, but the integer multiplication lowering uses unusual stride and subreg_offset combination breaks when one source is an attribute. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91970 Cc: "10.6 11.0" <mesa-stable@lists.freedesktop.org> Signed-off-by: Kristian Høgsberg Kristensen <krh@bitplanet.net> Reviewed-by: Matt Turner <mattst88@gmail.com> src/mesa/drivers/dri/i965/brw_fs.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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.