| Summary: | 4-27% performance drop in Vulkan benchmarks | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | Eero Tamminen <eero.t.tamminen> |
| Component: | Drivers/Vulkan/intel | Assignee: | Ian Romanick <idr> |
| Status: | VERIFIED FIXED | QA Contact: | Intel 3D Bugs Mailing List <intel-3d-bugs> |
| Severity: | normal | ||
| Priority: | medium | CC: | jason |
| Version: | unspecified | Keywords: | bisected, regression |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 109535, 109582 | ||
| Attachments: |
Avoid Vsync / Mailbox mode in fullscreen
Test case that reproduces the loop unrolling problem in GLSL. intel/compiler: Try nir_opt_if once before nir_opt_peephole_select |
||
|
Description
Eero Tamminen
2019-01-03 17:09:53 UTC
Ian: I wonder if that could be a result of this series : https://gitlab.freedesktop.org/mesa/mesa/merge_requests/9 (In reply to Lionel Landwerlin from comment #1) > Ian: I wonder if that could be a result of this series : > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/9 This series doesn't seem Vulkan specific. There was no drop in GL or GLES version of Aztec Ruins, only in Vulkan version of AztecRuins (*). And no drop in other GL benchmarks, so I think the regression is Vulkan specific. (*) While Vulkan version runs windowed (because GfxBench doesn't seem to support fullscreen for Vulkan), and GL & GLES versions are fullscreen, I don't think that test being windowed is relevant because: - the other regressing Vulkan tests are fullscreen - AztecRuins has low FPS Were you able to reproduce e.g. Deferred Multisampling Vulkan demo drop? (In reply to Eero Tamminen from comment #2) > (In reply to Lionel Landwerlin from comment #1) > > Ian: I wonder if that could be a result of this series : > > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/9 > > This series doesn't seem Vulkan specific. > > There was no drop in GL or GLES version of Aztec Ruins, only in Vulkan > version of AztecRuins (*). And no drop in other GL benchmarks, so I think > the regression is Vulkan specific. > > (*) While Vulkan version runs windowed (because GfxBench doesn't seem to > support fullscreen for Vulkan), and GL & GLES versions are fullscreen, I > don't think that test being windowed is relevant because: > - the other regressing Vulkan tests are fullscreen > - AztecRuins has low FPS > > Were you able to reproduce e.g. Deferred Multisampling Vulkan demo drop? Yep, bisecting now just to verify. Bisected to : commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 Author: Ian Romanick <ian.d.romanick@intel.com> Date: Tue May 22 18:56:41 2018 -0700 intel/compiler: More peephole select Shader-db results: The one shader hurt for instructions is a compute shader that had both spills and fills hurt. (In reply to Lionel Landwerlin from comment #4) > commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 > Author: Ian Romanick <ian.d.romanick@intel.com> > Date: Tue May 22 18:56:41 2018 -0700 > > intel/compiler: More peephole select > > Shader-db results: > > The one shader hurt for instructions is a compute shader that had both > spills and fills hurt. I think shaders in Sacha Willems' Vulkan demos would also need to be added to shader-db. According to INTEL_DEBUG output, Deferred Multisampling demo doesn't have any compute shaders or spills/fills, and it regressed most. (In reply to Lionel Landwerlin from comment #4) > Bisected to : > > commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 > Author: Ian Romanick <ian.d.romanick@intel.com> > Date: Tue May 22 18:56:41 2018 -0700 > > intel/compiler: More peephole select > > Shader-db results: > > The one shader hurt for instructions is a compute shader that had both > spills and fills hurt. This significantly impacts one a couple of the shaders of the multisampling demo : First one : Before: SIMD16 shader: 356 instructions. 0 loops. 1812 cycles. 0:0 spills:fills. Promoted 2 constants. Compacted 5696 to 3488 bytes (39%) After: SIMD16 shader: 157 instructions. 3 loops. 99970 cycles. 0:0 spills:fills. Promoted 2 constants. Compacted 2512 to 1536 bytes (39%) Second one: Before: SIMD16 shader: 249 instructions. 0 loops. 1170 cycles. 0:0 spills:fills. Promoted 0 constants. Compacted 3984 to 2560 bytes (36%) After: SIMD16 shader: 102 instructions. 3 loops. 4580 cycles. 0:0 spills:fills. Promoted 0 constants. Compacted 1632 to 1088 bytes (33%) This is one of the shader : https://github.com/SaschaWillems/Vulkan/blob/master/data/shaders/deferredmultisampling/deferred.frag (In reply to Eero Tamminen from comment #5) > (In reply to Lionel Landwerlin from comment #4) > > commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 > > Author: Ian Romanick <ian.d.romanick@intel.com> > > Date: Tue May 22 18:56:41 2018 -0700 > > > > intel/compiler: More peephole select > > > > Shader-db results: > > > > The one shader hurt for instructions is a compute shader that had both > > spills and fills hurt. > > I think shaders in Sacha Willems' Vulkan demos would also need to be added > to shader-db. According to INTEL_DEBUG output, Deferred Multisampling demo > doesn't have any compute shaders or spills/fills, and it regressed most. Totally, about to do that. (In reply to Lionel Landwerlin from comment #7) > (In reply to Eero Tamminen from comment #5) > > (In reply to Lionel Landwerlin from comment #4) > > > commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 > > > Author: Ian Romanick <ian.d.romanick@intel.com> > > > Date: Tue May 22 18:56:41 2018 -0700 > > > > > > intel/compiler: More peephole select > > > > > > Shader-db results: > > > > > > The one shader hurt for instructions is a compute shader that had both > > > spills and fills hurt. > > > > I think shaders in Sacha Willems' Vulkan demos would also need to be added > > to shader-db. According to INTEL_DEBUG output, Deferred Multisampling demo > > doesn't have any compute shaders or spills/fills, and it regressed most. > > Totally, about to do that. Branch here : https://gitlab.freedesktop.org/llandwerlin/shader-db/commits/master Need to fix the stub later. (In reply to Eero Tamminen from comment #5) > (In reply to Lionel Landwerlin from comment #4) > > commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 > > Author: Ian Romanick <ian.d.romanick@intel.com> > > Date: Tue May 22 18:56:41 2018 -0700 > > > > intel/compiler: More peephole select > > > > Shader-db results: > > > > The one shader hurt for instructions is a compute shader that had both > > spills and fills hurt. > > I think shaders in Sacha Willems' Vulkan demos would also need to be added > to shader-db. According to INTEL_DEBUG output, Deferred Multisampling demo > doesn't have any compute shaders or spills/fills, and it regressed most. Adding the GLSL shader won't actually help this problem. We'd have to add the SPIR-V instead. The issue is the weird way that glslang generates for-loops. I have a test case that reproduces the issue in plain GLSL. Created attachment 143043 [details]
Test case that reproduces the loop unrolling problem in GLSL.
There was also some discussion about the underlying issue in bug #109081. There is also https://github.com/airlied/pipeline-db to capture. For my own notes: The optimization that needs to be modified to also handle bcsel is opt_peel_loop_initial_if in nir_opt_if.c. Created attachment 143048 [details] [review] intel/compiler: Try nir_opt_if once before nir_opt_peephole_select Modifying opt_peel_loop_initial_if to work on bcsel in an manner similar to how it operates on if-statements (i.e., moving all of the instructions that are only used to support the sources of the bcsel to the end of the loop) is going to be non-trivial. In the mean time, this fixes the problem in the small test case that I supplied. As soon as it's done running through the CI, I will submit an MR. (In reply to Ian Romanick from comment #14) > Created attachment 143048 [details] [review] [review] > intel/compiler: Try nir_opt_if once before nir_opt_peephole_select This patch fixes the smaller perf regressions in (nbody & raytracing) compute tests, but it doesn't help at all with the largest regression, in Deferred Multisampling. (Aztec Ruins I couldn't test due to bug 109304.) (In reply to Eero Tamminen from comment #15) > (In reply to Ian Romanick from comment #14) > > Created attachment 143048 [details] [review] [review] [review] > > intel/compiler: Try nir_opt_if once before nir_opt_peephole_select > > This patch fixes the smaller perf regressions in (nbody & raytracing) > compute tests, but it doesn't help at all with the largest regression, in > Deferred Multisampling. It appears that the inner loop still isn't getting unrolled because of the problem this patch should have resolved. I'll keep digging. I put up a WIP: merge request. This series fixes the issue in the Sascha Willems demos for me. I get the same number of loops with this series as there was before 8fb8ebfbb05d. There's a one instruction regression in the deferredmultisampling demo compared to 8fb8ebfbb05d~1, but I can live with that. https://gitlab.freedesktop.org/mesa/mesa/merge_requests/107 I can't get a successful CI run even with Mesa master right now, so this is definitely not fully tested. (In reply to Ian Romanick from comment #17) > I put up a WIP: merge request. This series fixes the issue in the Sascha > Willems demos for me. I get the same number of loops with this series as > there was before 8fb8ebfbb05d. > > There's a one instruction regression in the deferredmultisampling demo > compared to 8fb8ebfbb05d~1, but I can live with that. > > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/107 > > I can't get a successful CI run even with Mesa master right now, so this is > definitely not fully tested. Your branch includes bug 109304 so it won't work with Aztec Ruins (which also regressed). If you can provide patch series I can apply on top of latest Mesa, I could do full test with it. Sorry, I somehow missed the download link (I haven't really looked at gitlab MRs before): https://gitlab.freedesktop.org/mesa/mesa/merge_requests/107.patch Performance-wise your changes mostly fix the regressing test-cases. There are still following gaps: * Deferred Multisampling: 0.5% (down from 27%) * Compute N-Body: 3% (down from 9%) Aztec Ruins and Compute Raytracing gaps seem to have been fixed to within daily variation. While I think this is fairly acceptable, could you check also Compute N-Body, why it still has a gap? (In reply to Eero Tamminen from comment #19) > Sorry, I somehow missed the download link (I haven't really looked at gitlab > MRs before): > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/107.patch > > Performance-wise your changes mostly fix the regressing test-cases. > > There are still following gaps: > * Deferred Multisampling: 0.5% (down from 27%) > * Compute N-Body: 3% (down from 9%) I was not able to reproduce this on my system. With system Mesa and the branch from the MR I get 34-35 fps. I looked at the assembly generated before 8fb8ebfbb05d and after this series, and, aside from slightly different register assignment, there's no difference. N-Body test has larger daily variance than the other tests. Looking at that for last ~3 month, it can be up to 3% i.e. the gap I saw. As you don't see any gap in this test, I guess I just got a kernel boot where allocation alignments happened so that perf was on the lower side of the variance. I.e. from my side, everything is good. I'll recheck the perf once the fix is in Mesa. The previous merge request had a few problems that caused a bunch of failures in Vulkan CTS. I have submitted a new MR that represents a slightly different approach. This seems to restore the performance of the Sascha Willems demos and passes Vulkan CTS. https://gitlab.freedesktop.org/mesa/mesa/merge_requests/170 I'm beginning to think we need to just revert the offending patch on the 19.0 branch. There are solutions to the problem but they involve either reworking NIR loops to have explicit continue blocks or very tricky optimizations. Neither of those (especially not the former) is something that I'd be super-excited to back-port. (In reply to Jason Ekstrand from comment #23) > I'm beginning to think we need to just revert the offending patch on the > 19.0 branch. There are solutions to the problem but they involve either > reworking NIR loops to have explicit continue blocks or very tricky > optimizations. Neither of those (especially not the former) is something > that I'd be super-excited to back-port. There is a mostly reviewed MR that I just need to finish. I'm really, sorry but since I've been back from FOSDEM, I've been very ill. If this was such a pressing issue for you, you could have also reviewed that patch series when I sent it out and tagged you on it. :( (In reply to Ian Romanick from comment #24) > There is a mostly reviewed MR that I just need to finish. I'm really, sorry > but since I've been back from FOSDEM, I've been very ill. If this was such > a pressing issue for you, you could have also reviewed that patch series > when I sent it out and tagged you on it. :( I'm sorry if my comments have been a bit curt and I haven't expressed my meaning fully. The difficulty for me isn't just the urgency (Vulkan basically has no loop unrolling right now) but that I'm not convinced the currently proposed fixes add up to a complete fix. What we had before worked and it did so very reliably as long as the loop contained no non-trivial continues. With 8fb8ebfbb05d33, loops now frequently end up with a different form that makes unrolling more difficult. Because of the nature of the change to the IR that can result from peephole select eating the continue block if statement, I'm not actually convinced that the sum total of the proposed fixes is enough to fix it in general. The proper fix for this is probably to do something smarter with continues in general. I'm not denying the utility of 8fb8ebfbb05d33; any patch which gets > 0.1% boost in shader-db is pretty impressive. I'm just claiming that there are clearly problems, things aren't baked well enough, and we may need to step back and try again. As a side-note, it's very clear that not having SPIR-V support in shader-db is a serious problem. There is vkpipeline-db but I'm not sure if that's really what we want as it records all the pipelines and doesn't do any de-duplication. Ian's patches were merged during weekend, and fix all the regressions indicated here. If we decide to revert the original patches from the 19.0 branch, I won't fight it. If that is ultimately the decision, the following commits will all need to be reverted, in this order: af07141b33d0a58ed2cfe915b95f146481a4ffef 378f9967710e9145f2a4f8eee89d87badbe0e6ea 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99 (In reply to Jason Ekstrand from comment #25) > As a side-note, it's very clear that not having SPIR-V support in shader-db > is a serious problem. There is vkpipeline-db but I'm not sure if that's > really what we want as it records all the pipelines and doesn't do any > de-duplication. Shouldn't it be possible to write a tool and add it to the vkpipeline-db project that compares shaders and removes duplicates? |
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.