<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>