On Fri, Oct 2, 2015 at 12:56 PM, Matt Turner <matts...@gmail.com> wrote: > On Fri, Oct 2, 2015 at 9:28 AM, Connor Abbott <cwabbo...@gmail.com> wrote: >> Some loops may have phi nodes that look like: >> >> foo = ... >> loop { >> bar = phi(foo, bar) >> ... >> } >> >> in which case we can remove the phi node and replace all uses of 'bar' >> with 'foo'. In particular, there are some L4D2 vertex shaders with loops >> that, after optimization, look like: >> >> /* succs: block_1 */ >> loop { >> block block_1: >> /* preds: block_0 block_4 */ >> vec1 ssa_2195 = phi block_0: ssa_2136, block_4: ssa_994 >> vec1 ssa_7321 = phi block_0: ssa_8195, block_4: ssa_7321 >> vec1 ssa_7324 = phi block_0: ssa_8198, block_4: ssa_7324 >> vec1 ssa_7327 = phi block_0: ssa_8174, block_4: ssa_7327 >> vec1 ssa_8139 = intrinsic load_uniform () () (232) >> vec1 ssa_588 = ige ssa_2195, ssa_8139 >> /* succs: block_2 block_3 */ >> if ssa_588 { >> block block_2: >> /* preds: block_1 */ >> break >> /* succs: block_5 */ >> } else { >> block block_3: >> /* preds: block_1 */ >> /* succs: block_4 */ >> } >> block block_4: >> /* preds: block_3 */ >> vec1 ssa_994 = iadd ssa_2195, ssa_2150 >> /* succs: block_1 */ >> } >> >> where after removing the second, third, and fourth phi nodes, the loop >> becomes >> entirely dead, and this patch will cause the loop to be deleted entirely. >> >> No piglit regressions. >> >> Shader-db results on bdw: >> >> instructions in affected programs: 5824 -> 5664 (-2.75%) >> helped: 32 >> HURT: 0 > > I added loop-count support to report.py recently. > > instructions in affected programs: 5824 -> 5664 (-2.75%) > total loops in shared programs: 2234 -> 2202 (-1.43%) > helped: 0 > > (report.py doesn't count programs in which the number of loops changed > as helped/hurt because sometimes a loop is unrolled and the > instruction count triples) > > I'd feel fine about changing helped: 0 to helped: 32 like you have. > >> >> Signed-off-by: Connor Abbott <cwabbo...@gmail.com> >> --- >> Seems like this one fell through the cracks... >> >> src/glsl/nir/nir_opt_remove_phis.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/src/glsl/nir/nir_opt_remove_phis.c >> b/src/glsl/nir/nir_opt_remove_phis.c >> index bf4a67e..0a5793a 100644 >> --- a/src/glsl/nir/nir_opt_remove_phis.c >> +++ b/src/glsl/nir/nir_opt_remove_phis.c >> @@ -58,6 +58,21 @@ remove_phis_block(nir_block *block, void *state) >> >> nir_foreach_phi_src(phi, src) { >> assert(src->src.is_ssa); >> + >> + /* For phi nodes at the beginning of loops, we may encounter some >> + * sources from backedges that point back to the destination of the >> + * same phi, i.e. something like: >> + * >> + * a = phi(a, b, ...) >> + * >> + * We can safely ignore these sources, since if all of the normal >> + * sources point to the same definition, then that definition must >> + * still dominate the phi node, and the phi will still always take >> + * the value of that definition. >> + */ >> + > > I'd remove the newline here. > >> + if (src->src.ssa == &phi->dest.ssa) >> + continue; >> >> if (def == NULL) { >> def = src->src.ssa; >> @@ -72,6 +87,11 @@ remove_phis_block(nir_block *block, void *state) >> if (!srcs_same) >> continue; >> >> + /* We must have found at least one definition, since there must be at >> + * least one forward edge. >> + */ >> + assert(def != NULL); >> + >> assert(phi->dest.is_ssa); >> nir_ssa_def_rewrite_uses(&phi->dest.ssa, nir_src_for_ssa(def)); >> nir_instr_remove(instr); >> -- >> 2.1.0 > > Look good to me. Thanks for remembering this. > > Reviewed-by: Matt Turner <matts...@gmail.com>
Ok, I fixed the newline and the results and pushed it. > > One strange thing I noticed... this doesn't affect any programs on > Haswell. All the shaders are vertex shaders, so they're vec4 on > Haswell but scalar on Broadwell, so it's probably somehow related to > that. > > Want to take a look at what's going on with Haswell? Seems like if the > loop is dead... it should be dead. Yeah, I noticed that too. I'll look into it... it could just be that the loops were getting deleted before this patch anyways without the scalarizer. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev