Bug 90520 - Register spilling clobbers registers used elsewhere in the shader
Summary: Register spilling clobbers registers used elsewhere in the shader
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
: 89786 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-05-19 13:40 UTC by Neil Roberts
Modified: 2015-09-03 00:43 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
INTEL_DEBUG=vs output (2.59 KB, text/plain)
2015-05-20 11:28 UTC, Neil Roberts
Details
INTEL_DEBUG=vs output (128.37 KB, text/plain)
2015-05-20 11:30 UTC, Neil Roberts
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Neil Roberts 2015-05-19 13:40:41 UTC
When a register is spilled it will emit a SHADER_OPCODE_GEN4_SCRATCH_WRITE pseudo opcode whenever it writes to the spilled register. When the code for this opcode is generated it will sneak in a write to MRF registers 14 and 15 in order to set up the message. On Gen7+ these MRF registers are mapped to GRF registers 126 and 127. The trouble is that the register allocator doesn't know that the pseudo opcode does these writes so it will happily allocate g126-g127 to something else and then it will get clobbered when the scratch write is done.

I noticed this because there are a bunch of varying-packing Piglit tests failing on Skylake. These get compiled very badly and end up using an excessive amount of virtual registers. Presumably the tests would also fail on Broadwell which also uses SIMD8 for vertex shaders. However I think the general problem presumably exists everywhere and even on Gen6 it might be possible to clobber an MRF register that is used elsewhere.

An example of a test that fails on Skylake is this:

piglit/bin/varying-packing-simple float array -auto -fbo
Comment 1 Jason Ekstrand 2015-05-20 00:38:11 UTC
(In reply to Neil Roberts from comment #0)
> When a register is spilled it will emit a SHADER_OPCODE_GEN4_SCRATCH_WRITE
> pseudo opcode whenever it writes to the spilled register. When the code for
> this opcode is generated it will sneak in a write to MRF registers 14 and 15
> in order to set up the message. On Gen7+ these MRF registers are mapped to
> GRF registers 126 and 127. The trouble is that the register allocator
> doesn't know that the pseudo opcode does these writes so it will happily
> allocate g126-g127 to something else and then it will get clobbered when the
> scratch write is done.

This seems fishy.  The allocator is supposed to detect that message registers are being used and reserve space at the top of the GRF for them.  This should trigger even if it's coming from the SHADER_OPCODE_GEN4_SCRATCH_WRITE opcode.  The one exception to this is the payload for the final FB-write which we allocate specially.

I did find one subtle bug in implied_mrf_writes() which I just sent a patch to fix.  However, I doubt that was enough to actually cause the problem you're describing.

Could you please attach the INTEL_DEBUG=fs dump?
Comment 2 Neil Roberts 2015-05-20 11:28:52 UTC
Created attachment 115923 [details]
INTEL_DEBUG=vs output

Note that the problem is with the vertex shader, not the fragment shader. Here is the INTEL_DEBUG=vs output. You can see on line 1953 it is trying to write to g127 in order to write it out to the URB on line 2150. However there are loads of register spills in between that overwrite g127 so it ends up writing garbage instead.

Note you can get this output on any platform like this:

INTEL_NO_HW=1 INTEL_DEVID_OVERRIDE=0x162e INTEL_DEBUG=vs piglit/bin/varying-packing-simple float array -auto -fbo

It looks like the implied_mrf_writes method is used by setup_mrf_hack_interference to make the MRF registers interfere with the corresponding GRF registers. However I don't think this is enough because there are no registers allocated for the message in SHADER_OPCODE_GEN4_SCRATCH_WRITE so there is nothing to check against for a collision.

I think for the other opcodes this works differently because for example the sampler instructions have the message registers as a source for the instruction so the allocator will know these registers are being used. However for the scratch write instruction the source is just an arbitrary register which gets implicitly copied into g127 behind the scenes.
Comment 3 Neil Roberts 2015-05-20 11:30:47 UTC
Created attachment 115924 [details]
INTEL_DEBUG=vs output

Oops, I accidentally sent the wrong file.
Comment 4 Neil Roberts 2015-05-27 16:02:01 UTC

*** This bug has been marked as a duplicate of bug 89786 ***
Comment 5 Jason Ekstrand 2015-06-06 16:44:47 UTC
I'm moving the duplicate status around.  Let's leave the descriptive one open.
Comment 6 Jason Ekstrand 2015-06-06 16:45:08 UTC
*** Bug 89786 has been marked as a duplicate of this bug. ***
Comment 7 Jason Ekstrand 2015-06-06 16:48:09 UTC
I did some poking around.  As far as I can tell, the MRF hack properly detects the implied writes and is setting register interferences for g126 and g127.  I wasn't able to get much further in my hour or so of poking yesterday.  Ken suggested that maybe it has something to do with one thing being a vector that only partially overlaps with the MRF hack.  I find that unlikely, but I guess it's possible.
Comment 8 Jason Ekstrand 2015-06-06 19:24:54 UTC
Patches sent.  It fixes it on my BDW.  Probably will for SKL as well.
Comment 9 Jim Bish 2015-06-12 19:10:09 UTC
confirmed on skl as well.
Comment 10 Matt Turner 2015-09-03 00:43:48 UTC
These patches were committed

commit 86e5afbfee5492235cab1a7be4ea49ac02be1644
Author: Jason Ekstrand <jason.ekstrand@intel.com>
Date:   Sat Jun 6 12:15:30 2015 -0700

    i965/fs: Don't let the EOT send message interfere with the MRF hack

commit 670862a5069f2759418450698aa4ab7d9f0e079f
Author: Jason Ekstrand <jason.ekstrand@intel.com>
Date:   Sat Jun 6 12:08:00 2015 -0700

    fs/reg_allocate: Remove the MRF hack helpers from fs_visitor

so presumably this is fixed.

Please leave the title of the patch(es) in the bug in the future.


bug/show.html.tmpl processed on Jan 18, 2017 at 01:44:53.
(provided by the Example extension).