Bug 98761 - [regression][radeonsi][polaris]"radeonsi: set IF_THRESHOLD to 3" breaks Witcher2's ground
Summary: [regression][radeonsi][polaris]"radeonsi: set IF_THRESHOLD to 3" breaks Witch...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/radeonsi (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact: Default DRI bug account
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-17 16:37 UTC by Arek Ruśniak
Modified: 2017-01-02 20:21 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
ground looks weird (595.46 KB, image/jpeg)
2016-11-17 16:37 UTC, Arek Ruśniak
Details
ground looks good (677.85 KB, image/jpeg)
2016-11-17 16:44 UTC, Arek Ruśniak
Details
problem in motion (6.92 MB, video/mp4)
2016-11-18 09:45 UTC, Arek Ruśniak
Details
affected shader (good asm) (180.49 KB, text/plain)
2016-11-22 00:36 UTC, Marek Olšák
Details
affected shader (good->bad diff) (16.81 KB, text/html)
2016-11-22 00:39 UTC, Marek Olšák
Details
affected shader (bad asm) (183.11 KB, text/plain)
2016-11-22 00:42 UTC, Marek Olšák
Details
still bad (68.22 KB, text/plain)
2016-11-24 19:57 UTC, Marek Olšák
Details
witcher2_r287854 (28.78 MB, video/x-matroska)
2016-11-24 23:20 UTC, Arek Ruśniak
Details

Description Arek Ruśniak 2016-11-17 16:37:01 UTC
Created attachment 128044 [details]
ground looks weird

mesa/llvm git/trunk
kernel 4.8.x & drm-next-4.10-wip

this is happend on polaris(rx470)
verde works corectly (for radeon drv, amdgpu not tested yet)


bisected:
74e39de9324d2d2333cda6adca50ae2a3fc36de2 is the first bad commit
commit 74e39de9324d2d2333cda6adca50ae2a3fc36de2
Author: Marek Olšák <marek.olsak@amd.com>
Date:   Fri Oct 28 23:08:50 2016 +0200

    radeonsi: set IF_THRESHOLD to 3
    
    Piglit regressions (radeonsi or LLVM bugs, they pass on softpipe):
    - glsl-1.10/execution/variable-indexing/vs-output-array-vec3-index-wr
    - glsl-1.10/execution/variable-indexing/vs-output-array-vec4-index-wr
    - glsl-110/execution/variable-indexing/vs-temp-array-mat2-index-col-row-wr
    - glsl-110/execution/variable-indexing/vs-temp-array-mat2-index-row-wr
    
    Totals:
    SGPRS: 1132185 -> 1168801 (3.23 %)
    VGPRS: 907856 -> 906204 (-0.18 %)
    Spilled SGPRs: 2011 -> 2425 (20.59 %)
    Spilled VGPRs: 368 -> 96 (-73.91 %)
    Scratch VGPRs: 1344 -> 1060 (-21.13 %) dwords per thread
    Code Size: 35916164 -> 35705372 (-0.59 %) bytes
    LDS: 767 -> 767 (0.00 %) blocks
    Max Waves: 194010 -> 194921 (0.47 %)
    Wait states: 0 -> 0 (0.00 %)
    
    Before:
     VGPR SPILLING APPS   Shaders SpillVGPR ScratchVGPR
     alien_isolation         2938        38        40
     bioshock-infinite       1769       245       732
     dirt-showdown            548        85        72
     f1-2015                  776         0       320
     ue4_lightroom_inter..     74         0       180
    
    After:
     VGPR SPILLING APPS   Shaders SpillVGPR ScratchVGPR
     alien_isolation         2938        38        40
     bioshock-infinite       1769         0       480
     dirt-showdown            548        58        40
     f1-2015                  776         0       320
     ue4_lightroom_inter..     74         0       180
    
    Bioshock and DiRT benefit.
    
    If I set IF_THRESHOLD=4, tesseract starts spilling VGPRs
    
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>

:040000 040000 2c773601e2956578e8da4a6031cb02837255b953 7d5f7c3faebe4a11631e70bde258d78fa7357008 M	src
Comment 1 Arek Ruśniak 2016-11-17 16:44:07 UTC
Created attachment 128045 [details]
ground looks good
Comment 2 Nicolai Hähnle 2016-11-17 22:15:26 UTC
The patch at https://patchwork.freedesktop.org/patch/122101/ fixes a known regression from that commit -- does it help here?
Comment 3 Arek Ruśniak 2016-11-18 09:45:29 UTC
Created attachment 128053 [details]
problem in motion

unfortunately, doesn't help here. (mesa git-da2a511 + your patch/llvm 287325)
this is how looks like in game before and after.
Comment 4 Marek Olšák 2016-11-18 17:53:26 UTC
I can reproduce it, but I don't yet know how to find the bad shader.

apitrace doesn't work with this game and shaders aren't always loaded in the same order.
Comment 5 Marek Olšák 2016-11-18 18:52:37 UTC
This is only reproducible on LLVM 4.0 trunk.

LLVM 3.9 renders doesn't have this issue.
Comment 6 Nicolai Hähnle 2016-11-18 20:24:45 UTC
If Witcher 2 has scalar spills, I'd suggest trying LLVM trunk from before r286766 (or perhaps with that commit reverted), as there's another known bug: SGPR spills conflict with fragment shader input interpolation (because both use M0), and it's possible that the Mesa commit simply exposed the issue.
Comment 7 Marek Olšák 2016-11-18 21:17:51 UTC
Found the bad shader.

The good one has before v_interp:
s_mov_b32 m0, s11

The bad one has before v_interp:
s_mov_b32 m0, s19

I don't see any store instructions though.
Comment 8 Marek Olšák 2016-11-18 21:19:27 UTC
Scratch that. The bad one has this:

s_mov_b32 s19, s11
s_mov_b32 m0, s19

It looks like it's not an m0 issue.
Comment 9 Daniel Scharrer 2016-11-19 05:39:17 UTC
Found a possibly related bug in another game: #98776.
Comment 10 Arek Ruśniak 2016-11-20 11:04:00 UTC
Nicolai, I don't want to bisect llvm I really hate it but i've tried r286757 & r286827 and problem is somewhere between. So you have right i believe. 

And still can't reproduce this on hd7770 (verde).
Comment 11 Marek Olšák 2016-11-21 23:52:43 UTC
First bad commit:

commit 4404d0d6e354e80dd7f8f0a0e12d8ad809cf007e
Author: Matt Arsenault <Matthew.Arsenault@amd.com>
Date:   Sun Nov 13 18:20:54 2016 +0000

    AMDGPU: Implement SGPR spilling with scalar stores
    
    nThis avoids the nasty problems caused by using
    memory instructions that read the exec mask while
    spilling / restoring registers used for control flow
    masking, but only for VI when these were added.
    
    This always uses the scalar stores when enabled currently,
    but it may be better to still try to spill to a VGPR
    and use this on the fallback memory path.
    
    The cache also needs to be flushed before wave termination
    if a scalar store is used.
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@286766 91177308-0d34-0410-b5e6-96231b3b80d8
Comment 12 Marek Olšák 2016-11-22 00:36:47 UTC
Created attachment 128128 [details]
affected shader (good asm)
Comment 13 Marek Olšák 2016-11-22 00:39:23 UTC
Created attachment 128129 [details]
affected shader (good->bad diff)
Comment 14 Marek Olšák 2016-11-22 00:42:20 UTC
Created attachment 128130 [details]
affected shader (bad asm)
Comment 15 Marek Olšák 2016-11-22 00:43:59 UTC
It looks like m0 isn't restored for v_interp instructions.
Comment 16 Grazvydas Ignotas 2016-11-22 12:54:33 UTC
Possible duplicates: bug 98776 bug 98783 bug 98785
llvm bug: https://llvm.org/bugs/show_bug.cgi?id=31019
Comment 17 Arek Ruśniak 2016-11-22 22:37:20 UTC
Marek, thanks for help with llvm. 
Grazvydas, with Talos I agree, even opengl is garbage now. Unigine-heaven has similar problem since r286766 
Probably guilty is "AMDGPU: Implement SGPR spilling with scalar stores" for all scenario but i never ever had any problem with Life is Strange (I've just finished E01)
Comment 18 Matt Arsenault 2016-11-24 00:40:34 UTC
Try after llvm r287844
Comment 19 Marek Olšák 2016-11-24 19:57:04 UTC
Created attachment 128178 [details]
still bad

(In reply to Matt Arsenault from comment #18)
> Try after llvm r287844

It's still pretty broken. See the attached asm. "s_mov vcc_hi, m0" looks suspicious.
Comment 20 Matt Arsenault 2016-11-24 20:53:44 UTC
(In reply to Marek Olšák from comment #19)
> Created attachment 128178 [details]
> still bad
> 
> (In reply to Matt Arsenault from comment #18)
> > Try after llvm r287844
> 
> It's still pretty broken. See the attached asm. "s_mov vcc_hi, m0" looks
> suspicious.

s_mov vcc_hi, m0 is fine and expected. TableGen happens to sort VCC to the beginning of the SReg_64 register class list, so the scavenger tends to find VCC first if its available
Comment 21 Matt Arsenault 2016-11-24 20:58:00 UTC
This is more concerning: s_buffer_store_dword exec_lo, s[84:87], m0

exec should never be spilled
Comment 22 Arek Ruśniak 2016-11-24 23:20:04 UTC
Created attachment 128180 [details]
witcher2_r287854

so, it's resolved ground's problem for me, the movie is not looking perfect but this is not problem of this bug report i think. 

Marek, Matt I don't know what do you talking about but maybe you are looking for:
https://bugs.freedesktop.org/show_bug.cgi?id=98238 
there is big visual differences between mesa 12.0.4 vs 13.0 ...work in progress:)
Comment 23 Arek Ruśniak 2017-01-02 20:21:31 UTC
Oops.. when Marek reverted "SGPR spilling..."-stuff, this one should be closed aswell.


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.