Bug 109081

Correction, test outputs here: 
https://mesa-ci.01.org/vulkancts/builds/9834/group/63a9f0ea7bb98050796b649e85481845#fails

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.

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. ;)

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.

(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()

(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?

(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.

(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?

(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.

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?

Yeah, it seems all of dEQP-VK.clipping.user_defined.* are passing on HSW with Mesa master.