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

[Thomas Helland]

2016-08-27 8:03 GMT+02:00 Timothy Arceri <timothy.arceri at collabora.com>:
> 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.

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