[Mesa-dev] [PATCH 06/12] nir: don't count removal of lcssa_phi as progress

[Timothy Arceri]

On Sat, 2016-08-27 at 16:06 +0200, Thomas Helland wrote:
> 2016-08-27 8:03 GMT+02:00 Timothy Arceri <timothy.arceri at collabora.co
> m>:
> > 
> > We generate these in each optimisation pass, counting them
> > as progress would cause the loop to run forever.
> 
> These are just some random thoughts that I thought I'd share.
> It might not make sense at all, and it might be answered in the
> patch series, but just thought I'd share them.
> 
> It's a bit unfortunate that we generate and remove phi's like this,
> but I'm not sure how it could be done in a better way.
> LCSSA, I think, is only useful if one is doing loop transformations.
> Maybe we could do loop transformations only once on the first
> iteration
> of the optimization loop, and then drop LCSSA transformation after
> that?
> Then again, future optimizations might open possibilities for
> unrolling,
> so this might not be ideal. There's also the issue that one might
> unroll
> a loop bit-by-bit on each iteration of the optimization loop, thereby
> completely unrolling a loop even if one only wants do to partial
> unrolling due to code size. I haven't studied how you hook the loop
> unrolling into the optimization loop, so my comments might not
> make sense at all.
> 
> > 
> > ---
> >  src/compiler/nir/nir_opt_remove_phis.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/compiler/nir/nir_opt_remove_phis.c
> > b/src/compiler/nir/nir_opt_remove_phis.c
> > index ee92fbe..13a139d 100644
> > --- a/src/compiler/nir/nir_opt_remove_phis.c
> > +++ b/src/compiler/nir/nir_opt_remove_phis.c
> > @@ -72,6 +72,7 @@ remove_phis_block(nir_block *block)
> >           break;
> > 
> >        nir_phi_instr *phi = nir_instr_as_phi(instr);
> > +      bool is_lcssa_phi = phi->is_lcssa_phi;
> > 
> >        nir_ssa_def *def = NULL;
> >        nir_alu_instr *mov = NULL;
> > @@ -118,6 +119,8 @@ remove_phis_block(nir_block *block)
> >        nir_instr_remove(instr);
> > 
> >        progress = true;
> > +      if (!is_lcssa_phi)
> > +         progress = true;
> 
> This does not do what you intended, as you do not remove
> the progress = true; line above the if you're inserting.
> 
> With that fixed, the patch itself looks good. It's kinda funny
> though that things presumable work even though this
> patch has no effect, indicating that it is not needed.

This looks like an error from rebasing. I believe it still works
because I added an is_simple_for_loop() check to the LCSSA pass so we
only do LCSSA if we are going to do unrolling which resolves the issues
you are talking about above. I should also be able add an
is_complex_for_loop() check once I add the support.

So in other words I think I can just drop this patch.


> 
> > 
> >     }
> > 
> >     return progress;
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev