[Mesa-dev] [PATCH] glsl: remove redundant function inout assignments

[Timothy Arceri]

On Tue, 2015-11-03 at 20:42 -0500, Ilia Mirkin wrote:
> On Tue, Nov 3, 2015 at 8:23 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > On Wed, 2015-11-04 at 12:12 +1100, Timothy Arceri wrote:
> > > On Tue, 2015-11-03 at 19:39 -0500, Ilia Mirkin wrote:
> > > > On Tue, Nov 3, 2015 at 7:31 PM, Timothy Arceri <t_arceri at yahoo.com.au>
> > > > wrote:
> > > > > On Tue, 2015-11-03 at 19:21 -0500, Ilia Mirkin wrote:
> > > > > > I'm still unclear what problem you're trying to solve here? What's
> > > > > > wrong with having b[1] = b[1]?
> > > > > 
> > > > > There is nothing wrong with it, but nir seems to expect this type of
> > > > > assignment to be optimised out and hits an assert if its not. Its
> > > > > removed
> > > > > for
> > > > > non arrays in opt_copy_propagation.cpp see add_copy()
> > > > 
> > > > Sounds like a bugfix for nir might be in order then? What if you have,
> > > > say, b[1] = b[1] + 0 and then NIR optimizes away the + 0? Or +
> > > > some_var which happens to become 0 as a result of optimizations?
> > > 
> > > Assuming the GLSL IR doesn't get to it first your right, however this is
> > > tripping up nir_lower_vars_to_ssa() which is the first optimisation call
> > > for
> > > nir drivers, so we still need to add this to the GLSL IR optimisations
> > > unless
> > > there is an argument to move the lower pass until after the other
> > > optimisation
> > > passes but I assume it was added to the beginning for a reason.
> > > 
> > > I moved the lower call to the end of the optimisation list to see what
> > > happens
> > > and it does indeed still fail. I can have a go at adding the
> > > optimisation to
> > > nir when I get some time
> > 
> > I can also note this in the bug report and leave it open until it is
> > resolved.
> > 
> > >  but I don't think this should be a blocker to the
> > > GLSL IR fix.
> 
> Perhaps you should wait for someone with a better understanding of
> what's going on than me to review.
> 
> It just seems odd to have this super-special-cased thing in a GLSL IR
> opt just because of some generic-looking NIR deficiency. It sounds
> like the sort of thing that'd be very easy to fix there, instead of
> adding seemingly unnecessary extra work in the GLSL IR.

The scenario this came up under was something like

for () {
   b[i] = b[i] * increamet_count; // asserts when increament_count == 1
}

Which is a little less super-special-case.

I'm all for not making our GLSL IR optimisation passes slower than they
already are but there is a lot worse things going on in those passes than
this, I fixed a couple recently but I'm sure there is a lot more low hanging
fruit in there if anyone really cared about speeding this stuff up.

> 
>   -ilia