<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 22, 2019 at 4:05 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 23/2/19 8:54 am, Jason Ekstrand wrote:<br>
> On Fri, Feb 22, 2019 at 2:47 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a> <br>
> <mailto:<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>>> wrote:<br>
> <br>
> <br>
> <br>
> On 23/2/19 6:31 am, Rob Clark wrote:<br>
> > On Fri, Feb 22, 2019 at 12:39 PM Jason Ekstrand<br>
> <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a> <mailto:<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>>> wrote:<br>
> >><br>
> >> On Fri, Feb 22, 2019 at 9:51 AM Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a><br>
> <mailto:<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>>> wrote:<br>
> >>><br>
> >>> Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a><br>
> <mailto:<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>>> writes:<br>
> >>><br>
> >>>> shader-db results i965 (SKL):<br>
> >>>><br>
> >>>> total instructions in shared programs: 13219105 -> 13024761<br>
> (-1.47%)<br>
> >>>> instructions in affected programs: 1169457 -> 975113 (-16.62%)<br>
> >>>> helped: 599<br>
> >>>> HURT: 154<br>
> >>>><br>
> >>>> total cycles in shared programs: 333968972 -> 324822073 (-2.74%)<br>
> >>>> cycles in affected programs: 130032440 -> 120885541 (-7.03%)<br>
> >>>> helped: 590<br>
> >>>> HURT: 216<br>
> >>>><br>
> >>>> total spills in shared programs: 57947 -> 29130 (-49.73%)<br>
> >>>> spills in affected programs: 53364 -> 24547 (-54.00%)<br>
> >>>> helped: 351<br>
> >>>> HURT: 0<br>
> >>>><br>
> >>>> total fills in shared programs: 51310 -> 25468 (-50.36%)<br>
> >>>> fills in affected programs: 44882 -> 19040 (-57.58%)<br>
> >>>> helped: 351<br>
> >>>> HURT: 0<br>
> >>>> ---<br>
> >>>> src/compiler/nir/nir_lower_phis_to_scalar.c | 1 +<br>
> >>>> 1 file changed, 1 insertion(+)<br>
> >>>><br>
> >>>> diff --git a/src/compiler/nir/nir_lower_phis_to_scalar.c<br>
> b/src/compiler/nir/nir_lower_phis_to_scalar.c<br>
> >>>> index 16001f73685..f6f702bca15 100644<br>
> >>>> --- a/src/compiler/nir/nir_lower_phis_to_scalar.c<br>
> >>>> +++ b/src/compiler/nir/nir_lower_phis_to_scalar.c<br>
> >>>> @@ -74,6 +74,7 @@ is_phi_src_scalarizable(nir_phi_src *src,<br>
> >>>> /* A phi is scalarizable if we're going to lower it */<br>
> >>>> return should_lower_phi(nir_instr_as_phi(src_instr),<br>
> state);<br>
> >>>><br>
> >>>> + case nir_instr_type_tex:<br>
> >>>> case nir_instr_type_load_const:<br>
> >>>> case nir_instr_type_ssa_undef:<br>
> >>>> /* These are trivially scalarizable */<br>
> >>><br>
> >>> Sounds promising, but I would definitely not describe<br>
> instr_type_tex as<br>
> >>> "trivially scalarizable" -- could you explain what's going on<br>
> with this<br>
> >>> patch?<br>
> <br>
> Basically it just turns:<br>
> <br>
> if ssa0 {<br>
> ...<br>
> vec4 ssa1 = txf .....<br>
> } else {<br>
> ...<br>
> vec4 ssa2 = ...<br>
> }<br>
> vec4 ss3 = phi ssa1, ssa2<br>
> <br>
> Into<br>
> <br>
> if ssa0 {<br>
> ...<br>
> vec4 ssa1 = txf .....<br>
> vec1 ssa2 = imov ssa1.x<br>
> vec1 ssa3 = imov ssa1.y<br>
> vec1 ssa4 = imov ssa1.z<br>
> vec1 ssa5 = imov ssa1.w<br>
> } else {<br>
> ...<br>
> vec4 ssa6 = ...<br>
> vec1 ssa7 = imov ssa6.x<br>
> vec1 ssa8 = imov ssa6.y<br>
> vec1 ssa9 = imov ssa6.z<br>
> vec1 ssa10 = imov ssa6.w<br>
> }<br>
> vec1 ss11 = phi ssa2, ssa7<br>
> vec1 ss12 = phi ssa3, ssa8<br>
> vec1 ss13 = phi ssa4, ssa9<br>
> vec1 ss14 = phi ssa5, ssa10<br>
> <br>
> <br>
> This allows a whole bunch more optimisation to take place as often not<br>
> all of the phi channels are actually used. If some cases large<br>
> chunks of<br>
> logic can be remove from the if branch that doesn't contain the texture<br>
> access.<br>
> <br>
> >><br>
> >><br>
> >> I think I can for Intel though I'm not sure how this affects<br>
> other drivers.<br>
> >><br>
> >> On Intel hardware, we almost always have to combine all the<br>
> texture sources into one big message. Since having more than one<br>
> source is very common, this means that we have to make a temporary<br>
> copy of the sources anyway. Because we're copying them, having them<br>
> contiguous (a vector in NIR terms) doesn't actually gain us<br>
> anything. We may as well let NIR scalarize them and give more<br>
> freedom to the register allocator and other NIR passes which may<br>
> need to clean things up. We don't want to make the same choice for<br>
> destinations as they are required to be contiguous.<br>
> >><br>
> ><br>
> > hmm, but this is abut the phi src (ie. the tex dest), not the tex<br>
> src, isn't it?<br>
> <br>
> <br>
> Well, that's a pickle... When I wrote this pass I did so to try and <br>
> explicitly keep from breaking up things that are known to be vectors in <br>
> the back-end such as textures. The idea was to try and not break up <br>
> things like this:<br>
> <br>
> if (...) {<br>
> ssa_1 = tex()<br>
> } else {<br>
> ssa_2 = tex()<br>
> }<br>
> ssa_3 = phi (ssa_1, ssa_2)<br>
> <br>
> in the hopes that it would turn into<br>
> <br>
> if (...) {<br>
> r1 = tex();<br>
> } else {<br>
> r1 = tex();<br>
> }<br>
> <br>
> Clearly, that notion was mis-placed. At this point, I really wonder <br>
> what the complexity is saving us. Maybe it's not worth it at all? <br>
> Maybe we need to be more agressive and require all sources to not be <br>
> vectorizable or something?<br>
<br>
Maybe checking if all components of the phi are used before converting <br>
to scalar would be useful? Not sure but I've sent a more conservative v2 <br>
of this patch where a bunch of hurt is gone.<br>
<br>
The remaining hurt is very small and comes from shaders like this:<br>
<br>
if (...) {<br>
r1 = tex();<br>
} else {<br>
if (...) {<br>
r1 = tex();<br>
} else {<br>
r1 = undefined;<br>
}<br>
}<br></blockquote><div><br></div><div>We generally shouldn't let undef affect things. This is currently true but not anymore with your new heuristic. I'm thinking about how to actually write the heuristic I have in my head and it's really annoying. :'( <br></div></div></div>