<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 14, 2016 at 1:50 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, 2016-04-13 at 08:29 -0700, Jason Ekstrand wrote:<br>
><br>
> On Apr 13, 2016 7:57 AM, "Connor Abbott" <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> wrote:<br>
> ><br>
> > On Wed, Apr 13, 2016 at 3:24 AM, Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>><br>
> wrote:<br>
> > > On Tue, 2016-04-12 at 13:16 -0400, Connor Abbott wrote:<br>
> > >> I'm not sure I'm so comfortable with this. For one, this is the<br>
> sort<br>
> > >> of thing the backend can (and should) do. Why should be be adding<br>
> > >> stuff at the NIR level to help the backend's copy propagation<br>
> pass?<br>
> > >> And perhaps more importantly, why should we be adding stuff at<br>
> the NIR<br>
> > >> level to workaround the lack of spilling support for doubles? I<br>
> know<br>
> > >> it's more difficult to implement than normal spilling, but it<br>
> just<br>
> > >> feels dirty to workaround it because some tests happen to use too<br>
> many<br>
> > >> registers.<br>
> > ><br>
> > > My thoughts for this were that NIR is producing suboptimal output<br>
> for<br>
> > > this case: it is unpacking and repacking an entire 64-bit float<br>
> when it<br>
> > > only really needs to operate on the high 32-bit, so fixing that in<br>
> NIR,<br>
> > > where the issue is introduced, instead of having each backend fix<br>
> it,<br>
> > > seemed to make more sense to me.<br>
> > ><br>
> > > I am not sure that backends can easily fix this either: what they<br>
> are<br>
> > > going to see is a nir_op_pack_double_2x32_split operation, they<br>
> don't<br>
> > > have any context to tell if the low 32-bit argument is actually<br>
> > > different from what is already in the register, other than<br>
> emitting code<br>
> > > to check for that which kind of defeats the purpose of the<br>
> optimization.<br>
> > > I suppose they could still see if the instruction that generated<br>
> the low<br>
> > > 32-bit value is actually an unpack of the same register and do the<br>
> right<br>
> > > thing in that case. Would you like that solution better?<br>
> > ><br>
> > > In any case, I don't think this is something for copy-propagation<br>
> to<br>
> > > handle this is really not about copy-propagating a value, it is<br>
> about<br>
> > > not needing that value to begin with. Register pressure comes from<br>
> > > needing to have the low 32-bit value alive until we recombine it<br>
> in the<br>
> > > 64-bit value. This patch makes it so that the low 32-bit is not<br>
> alive at<br>
> > > any moment, we don't need to care about it at any point, and that<br>
> is why<br>
> > > it helps reduce register pressure.<br>
><br>
> I think we have two fundamental problems here.  One is that spilling<br>
> is broken.  The second is that NIR is generating code that the backend<br>
> can't handle without spilling.  I can totally believe that the later<br>
> is happening given that, thanks to the flag, a the backend can't move<br>
> two SELs past each other when it schedules and we use SEL very heavily<br>
> in the double lowering.<br>
><br>
> In this particular case, I do think we have a coalescing problem more<br>
> than a need for a new opcode.  Thanks to SSA you could just as easily<br>
> see a pack where one source is an unpack and do what you're doing with<br>
> this "replace the top bits" opcode in fs/vec4_nir without adding a<br>
> special opcode for it.  Therefore, even if this little optimization<br>
> helps, I think the opcode is unneeded.  I also think it's probably<br>
> solvable as Connor says with better coalescing.<br>
<br>
</div></div>Yes, this is what I suggested as an alternative to do this in the<br>
backend in my original reply to Connor. There is a small caveat here<br>
though: we would have to assume that if the low 32-bit operand comes<br>
from an unpack opcode, it is an unpack of the same value. We can't<br>
verify this precisely because of SSA.</blockquote><div><br></div><div>I don't follow.  If you have<br><br></div><div>ssa_1 = unpack(ssa_0)<br></div><div>ssa_2 = stuff<br></div><div>ssa_3 = pack_split(ssa_2, ssa_1.y)<br><br></div><div>that should be exactly equivalent to<br><br></div><div>ssa_2 = stuff<br></div><div>ssa_3 = pack_split_y(ssa_0, ssa_2)<br><br></div><div>You can easily detect it and just do the slightly simpler thing.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It is probably okay to assume<br>
this, since I don't imagine situations where we want to unpack a double<br>
and repack its low 32-bit into a different double value, but it is a bit<br>
of a hack :)<br></blockquote><div><br></div><div>Sounds like we have hacks all round :-)<br><br></div><div>I don't know if you're doing this, but one thing that might help substantially with your spilling problems is if we run lower_alu_to_scalar before we lower double ops.<br><br></div><div>Is there a branch I can play with that has these spilling problems?  If so, what tests do I need to look at?  I'm planning to start playing with making spilling work better and would like to fix it for the fp64 case while I'm at it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> > Think of what the backend is going to turn this into, though. Say<br>
> you<br>
> > have a sequence like:<br>
> ><br>
> > hi = unpack_double_2x32_split_y foo<br>
> ><br>
> > new_hi = ...<br>
> ><br>
> > lo = unpack_double_2x32_split_x foo<br>
> > bar = pack_double_2x32_split lo, new_hi<br>
> ><br>
> > then this gets transformed to, in the backend:<br>
> ><br>
> > hi = foo (stride 2, offset 1)<br>
> ><br>
> > new_hi  = ..<br>
> ><br>
> > lo = foo (stride 2, offset 0)<br>
> > bar (stride 2, offset 0) = lo (stride 2, offset 0) # <- notice this<br>
> > bar (stride 2, offset 1) hi (stride 2, offset 1)<br>
> ><br>
> > Now the move from lo to bar has the same stride, and the same<br>
> offset,<br>
> > and the same offset, and bar obviously doesn't conflict with lo, so<br>
> > register coalescing can combine lo and bar. This will result in:<br>
> ><br>
> > hi = foo (stride 2, offset 1)<br>
> ><br>
> > new_hi  = ..<br>
> ><br>
> > bar (stride 2, offset 0) = foo (stride 2, offset 0)<br>
> > bar (stride 2, offset 1) hi (stride 2, offset 1)<br>
> ><br>
> > This is the same code (with the same register pressure) as if you<br>
> had<br>
> > used pack_double_2x32_split_y. In fact, in the bugzilla bug you gave<br>
> > me an example of a shader where this happens, and I pointed it out<br>
> to<br>
> > you. I think all you need to do to fix your problem is move down the<br>
> > unpack_2x32_split_x to right before the pack, since the backend<br>
> isn't<br>
> > smart enough to reorder it.<br>
> ><br>
> > In any case, I think that we should get spilling to work on doubles<br>
> regardless.<br>
> ><br>
> > ><br>
> > > Jason also mentioned that what we should do is fix the spilling<br>
> code. I<br>
> > > agree that we should do that, but I think the underlying issue<br>
> here is<br>
> > > not that, but that NIR's output is leading to inefficient code,<br>
> which is<br>
> > > then leading to unnecessary spilling in some cases. If we fix the<br>
> > > spilling then we can live with the problem, but the problem is<br>
> still<br>
> > > there: we are still having live values that we don't need taking<br>
> some<br>
> > > register space and leading to unnecessary spilling.<br>
> > ><br>
> > > In any case, I understand why you don't like the patch, so I am<br>
> fine<br>
> > > with holding it back and focus on fixing spilling code first.<br>
> FWIW, I<br>
> > > already had a stab at that, but I run into something that I didn't<br>
> quite<br>
> > > figure out at that time:<br>
> > ><br>
> > ><br>
> <a href="https://lists.freedesktop.org/archives/mesa-dev/2016-March/109370.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/mesa-dev/2016-March/109370.html</a><br>
> ><br>
> > I didn't notice this fly by, but you're not doing the spilling<br>
> right.<br>
> > For things where the stride * type_size is 64, you can't decompose<br>
> it<br>
> > into two scratch reads/writes, since then the exec mask will be<br>
> wrong.<br>
> > You only want to read/write a channel if the exec mask is enabled<br>
> for<br>
> > it. You have to use a different message to spill doubles, since the<br>
> > normal one (the scattered read/write) assumes the exec mask channels<br>
> > correspond to 32-bit chunks. Jason would know more about this.<br>
> ><br>
> > ><br>
> > > I still have plans to get back to that though.<br>
> > ><br>
> > > Iago<br>
> > ><br>
> > >> Jason, what do you think?<br>
> > >><br>
> > >> On Tue, Apr 12, 2016 at 4:05 AM, Samuel Iglesias Gonsálvez<br>
> > >> <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>> wrote:<br>
> > >> > From: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>><br>
> > >> ><br>
> > >> > This is useful when we only need to modify the high 32-bit<br>
> chunk of a double.<br>
> > >> > This is a common case, because this is the part that encodes<br>
> the exponent<br>
> > >> > which we manipulate in some double lowering passes. Although we<br>
> can accomplish<br>
> > >> > the same by using pack_double_2x32, this new opcode is better<br>
> for register<br>
> > >> > pressure, since we don't have to unpack both parts of the<br>
> double and keep<br>
> > >> > the low 32-bits around until we can recombine the new exponent.<br>
> > >> ><br>
> > >> > We will use this opcode in the set_exponent() function of the<br>
> double lowering<br>
> > >> > pass and with that we will fix spilling issues in some dmat4<br>
> divide<br>
> > >> > piglit tests on Intel.<br>
> > >> > ---<br>
> > >> >  src/compiler/nir/nir_opcodes.py | 14 ++++++++++++++<br>
> > >> >  1 file changed, 14 insertions(+)<br>
> > >> ><br>
> > >> > diff --git a/src/compiler/nir/nir_opcodes.py<br>
> b/src/compiler/nir/nir_opcodes.py<br>
> > >> > index 9f62e08..f92dd8d 100644<br>
> > >> > --- a/src/compiler/nir/nir_opcodes.py<br>
> > >> > +++ b/src/compiler/nir/nir_opcodes.py<br>
> > >> > @@ -276,6 +276,20 @@ di.i2 = src0.y;<br>
> > >> >  dst.x = di.u64;<br>
> > >> >  """)<br>
> > >> ><br>
> > >> > +opcode("pack_double_2x32_split_y", 0, tuint64, [0, 0],<br>
> [tuint64, tuint32], "", """<br>
> > >> > +union {<br>
> > >> > +  Â  uint64_t u64;<br>
> > >> > +  Â  struct {<br>
> > >> > +  Â  Â  Â  uint32_t i1;<br>
> > >> > +  Â  Â  Â  uint32_t i2;<br>
> > >> > +  Â  };<br>
> > >> > +} di;<br>
> > >> > +<br>
> > >> > +di.u64 = src0;<br>
> > >> > +di.i1 = src1;<br>
> > >> > +dst = di.u64;<br>
> > >> > +""")<br>
> > >> > +<br>
> > >> >  unop_horiz("unpack_double_2x32", 2, tuint32, 1, tuint64, """<br>
> > >> >  union {<br>
> > >> >  Â  Â  uint64_t u64;<br>
> > >> > --<br>
> > >> > 2.5.0<br>
> > >> ><br>
> > >> > _______________________________________________<br>
> > >> > mesa-dev mailing list<br>
> > >> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > >> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
> > >> _______________________________________________<br>
> > >> mesa-dev mailing list<br>
> > >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
> > ><br>
> > ><br>
><br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>