Bug 109081

Summary: [bisected] [HSW] Regression in clipping.user_defined.clip_* vulkancts tests
Product: Mesa Reporter: Clayton Craft <clayton.a.craft>
Component: Drivers/DRI/i965Assignee: Ian Romanick <idr>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: idr, jason, kenneth, t_arceri
Version: gitKeywords: bisected
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Shader that exhibits the loop unrolling problem.

Description Clayton Craft 2018-12-18 00:04:27 UTC
The following tests have regressed on vulkancts/HSW:


dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.1_7
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.2_6
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.3_5
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.4_4
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.5_3
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.6_2
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.7_1
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.8
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.1_7
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.2_6
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.3_5
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.4_4
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.5_3
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.6_2
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.7_1
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess_geom.8
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.1
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.2
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.3
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.4
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.5
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.6
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.7
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.8
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.1
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.2
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.3
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.4
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.5
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.6
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.7
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess_geom.8


Test outputs can be viewed here:

http://mesa-ci-results.jf.intel.com/vulkancts/builds/9834/group/63a9f0ea7bb98050796b649e85481845#fails





Bisected to this commit:

commit 8fb8ebfbb05d3323481c8ba6d320b3a3580bad99
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Tue May 22 18:56:41 2018 -0700

    intel/compiler: More peephole select
Comment 1 Clayton Craft 2018-12-18 01:42:36 UTC
Correction, test outputs here: 
https://mesa-ci.01.org/vulkancts/builds/9834/group/63a9f0ea7bb98050796b649e85481845#fails
Comment 2 Ian Romanick 2018-12-18 02:17:34 UTC
Ken and I looked at 
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.1_7.  There are two problems that appear to just be uncovered by this commit.

1. A loop that was previously unrolled is no longer unrolled.  There is code in nir_loop_analysis to handle cases like

    ssa_50 = phi ssa_0, ssa_54

    if ssa_52 {
        ssa_53 = iadd ssa_50, ssa_1
    }
    ssa_54 = phi ssa_50, ssa_53

but it does not appear handle

    ssa_50 = phi ssa_0, ssa_54
    ssa_53 = iadd ssa_50, ssa_1
    ssa_54 = bcsel ssa_52, ssa_53, ssa_50

2. There is a local, compact array that does not get lowered by nir_lower_indirect_derefs.  When the loop is unrolled, the temporary array is eliminated, so this problem is not observable.
Comment 3 Ian Romanick 2018-12-18 08:23:21 UTC
I've submitted an MR for a possible fix for problem #2.

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/26

I'm have an idea for problem #1, but I don't know that I'll get to it before the new year.  Maybe Tim will beat me to it. ;)
Comment 4 Ian Romanick 2018-12-18 08:24:47 UTC
Created attachment 142844 [details]
Shader that exhibits the loop unrolling problem.

This shader exhibits the unrolling problem.  Before the bisected commit, the loop unrolls.  After the bisected commit it does not.
Comment 5 Timothy Arceri 2018-12-18 09:11:27 UTC
(In reply to Ian Romanick from comment #3)
> I've submitted an MR for a possible fix for problem #2.
> 
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/26
> 
> I'm have an idea for problem #1, but I don't know that I'll get to it before
> the new year.  Maybe Tim will beat me to it. ;)

The problem with peephole select is that we loose opportunities to apply the growing list of opts in nir_opt_if(). In this case we no longer recognise the pattern in opt_peel_loop_initial_if()
Comment 6 Ian Romanick 2018-12-18 22:06:46 UTC
(In reply to Timothy Arceri from comment #5)
> (In reply to Ian Romanick from comment #3)
> > I've submitted an MR for a possible fix for problem #2.
> > 
> > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/26
> > 
> > I'm have an idea for problem #1, but I don't know that I'll get to it before
> > the new year.  Maybe Tim will beat me to it. ;)
> 
> The problem with peephole select is that we loose opportunities to apply the
> growing list of opts in nir_opt_if(). In this case we no longer recognise
> the pattern in opt_peel_loop_initial_if()

I suspected it was something like that.  I was planning to go through and beef up the places that handle if-statements to also handle bcsel.  That doesn't (conceptually) seem to hard.  What do you think?
Comment 7 Timothy Arceri 2018-12-18 22:38:25 UTC
(In reply to Ian Romanick from comment #6)
> (In reply to Timothy Arceri from comment #5)
> > (In reply to Ian Romanick from comment #3)
> > > I've submitted an MR for a possible fix for problem #2.
> > > 
> > > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/26
> > > 
> > > I'm have an idea for problem #1, but I don't know that I'll get to it before
> > > the new year.  Maybe Tim will beat me to it. ;)
> > 
> > The problem with peephole select is that we loose opportunities to apply the
> > growing list of opts in nir_opt_if(). In this case we no longer recognise
> > the pattern in opt_peel_loop_initial_if()
> 
> I suspected it was something like that.  I was planning to go through and
> beef up the places that handle if-statements to also handle bcsel.  That
> doesn't (conceptually) seem to hard.  What do you think?

It should be fine for opt_peel_loop_initial_if() but for things like opt_if_evaluate_condition_use() and https://patchwork.freedesktop.org/patch/263442/ it's not really possible to do.
Comment 8 Kenneth Graunke 2018-12-19 10:49:36 UTC
(In reply to Timothy Arceri from comment #7)
> > I suspected it was something like that.  I was planning to go through and
> > beef up the places that handle if-statements to also handle bcsel.  That
> > doesn't (conceptually) seem to hard.  What do you think?
> 
> It should be fine for opt_peel_loop_initial_if() but for things like
> opt_if_evaluate_condition_use() and
> https://patchwork.freedesktop.org/patch/263442/ it's not really possible to
> do.

If that's the case, then maybe we ought to be doing peephole select optimizations (or at least the fancier versions?) later in the optimization process, after we do those other passes to clean up control flow?
Comment 9 Timothy Arceri 2018-12-19 11:09:23 UTC
(In reply to Kenneth Graunke from comment #8)
> (In reply to Timothy Arceri from comment #7)
> > > I suspected it was something like that.  I was planning to go through and
> > > beef up the places that handle if-statements to also handle bcsel.  That
> > > doesn't (conceptually) seem to hard.  What do you think?
> > 
> > It should be fine for opt_peel_loop_initial_if() but for things like
> > opt_if_evaluate_condition_use() and
> > https://patchwork.freedesktop.org/patch/263442/ it's not really possible to
> > do.
> 
> If that's the case, then maybe we ought to be doing peephole select
> optimizations (or at least the fancier versions?) later in the optimization
> process, after we do those other passes to clean up control flow?

I tried that but last time I checked it resulted in more hurt then help, like most optimisations its a bit of a trade off when and where its called.
Comment 10 Juan A. Suarez 2018-12-27 16:44:59 UTC
There was a revert in master, 29e4b949b4 ("Revert "nir/lower_indirect: Bail early if modes == 0""), that apparently fixes this bug.

If so, can we close this as FIXED?
Comment 11 Tapani Pälli 2019-01-02 10:53:50 UTC
Yeah, it seems all of dEQP-VK.clipping.user_defined.* are passing on HSW with Mesa master.

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.