<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, May 26, 2018 at 11:10 AM, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</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">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> From: Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>><br>
><br>
> When we don't have PLN (gen4 and gen11+), we implement LINTERP as either<br>
> LINE+MAC or a pair of MADs.  In both cases, the accumulator is written<br>
> by the first of the two instructions and read by the second.  Even<br>
> though the accumulator value isn't actually ever used from a logical<br>
> instruction perspective, it is trashed so we need to make the scheduler<br>
> aware.  Otherwise, the scheduler could end up re-ordering instructions<br>
> and putting a LINTERP between another an instruction which writes the<br>
> accumulator and another which tries to use that result.<br>
><br>
> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
> ---<br>
>  src/intel/compiler/brw_shader.<wbr>cpp | 3 ++-<br>
>  1 file changed, 2 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/intel/compiler/brw_<wbr>shader.cpp b/src/intel/compiler/brw_<wbr>shader.cpp<br>
> index 141b64e..dfd2c5c 100644<br>
> --- a/src/intel/compiler/brw_<wbr>shader.cpp<br>
> +++ b/src/intel/compiler/brw_<wbr>shader.cpp<br>
> @@ -984,7 +984,8 @@ backend_instruction::writes_<wbr>accumulator_implicitly(const struct gen_device_info<br>
>     return writes_accumulator ||<br>
>            (devinfo->gen < 6 &&<br>
>             ((opcode >= BRW_OPCODE_ADD && opcode < BRW_OPCODE_NOP) ||<br>
> -            (opcode >= FS_OPCODE_DDX_COARSE && opcode <= FS_OPCODE_LINTERP)));<br>
> +            (opcode >= FS_OPCODE_DDX_COARSE && opcode <= FS_OPCODE_LINTERP))) ||<br>
> +          (opcode == FS_OPCODE_LINTERP && !devinfo->has_pln);<br>
<br>
</div></div>The devinfo->has_pln condition is technically inaccurate, because even<br>
SNB will fall back to the non-PLN path which overwrites the accumulator<br>
for certain valid IR, which yeah I'm aware is not *typically*<br>
encountered before this series,</blockquote><div><br></div><div>I'm pretty sure it's impossible before this series because, without SIMD32, the barycentric coordinates always start at g2 and get incremented by 2 every time.  The only other way to get something into the coordinates source of the PLN is with a pixel interpolator message.  For that, the register allocator has a workaround to ensure that it's assigned an even register on SNB.  One of the early patches in my series replaces the broken gen4.5-6 PLN fall-back (it didn't work in SIMD16 because it assumed the wrong register layout for coordinates) with an assert and Jenkins is just fine with the assert.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">but why make things more inaccurate than<br>
the original only to revert back to a devinfo->gen based check in<br>
PATCHv2 33?<br></blockquote><div><br></div><div>See above.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think I'd squash the last hunk of PATCHv2 33 into this one which would<br>
give you something functionally equivalent to v1 but updated to handle<br>
Gen11 correctly.<br>
<br>
>  }<br>
>  <br>
>  bool<br>
<span class="HOEnZb"><font color="#888888">> -- <br>
> 2.5.0.400.gff86faf<br>
</font></span></blockquote></div><br></div></div>