2016-08-27 8:03 GMT+02:00 Timothy Arceri <timothy.arc...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
