[Mesa-dev] [PATCH v2 32/53] intel/fs: Mark LINTERP opcode as writing accumulator on platforms without PLN

[Jason Ekstrand]

On Sat, May 26, 2018 at 11:10 AM, Francisco Jerez <currojerez at riseup.net>
wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > From: Francisco Jerez <currojerez at riseup.net>
> >
> > When we don't have PLN (gen4 and gen11+), we implement LINTERP as either
> > LINE+MAC or a pair of MADs.  In both cases, the accumulator is written
> > by the first of the two instructions and read by the second.  Even
> > though the accumulator value isn't actually ever used from a logical
> > instruction perspective, it is trashed so we need to make the scheduler
> > aware.  Otherwise, the scheduler could end up re-ordering instructions
> > and putting a LINTERP between another an instruction which writes the
> > accumulator and another which tries to use that result.
> >
> > Cc: mesa-stable at lists.freedesktop.org
> > ---
> >  src/intel/compiler/brw_shader.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/compiler/brw_shader.cpp b/src/intel/compiler/brw_
> shader.cpp
> > index 141b64e..dfd2c5c 100644
> > --- a/src/intel/compiler/brw_shader.cpp
> > +++ b/src/intel/compiler/brw_shader.cpp
> > @@ -984,7 +984,8 @@ backend_instruction::writes_accumulator_implicitly(const
> struct gen_device_info
> >     return writes_accumulator ||
> >            (devinfo->gen < 6 &&
> >             ((opcode >= BRW_OPCODE_ADD && opcode < BRW_OPCODE_NOP) ||
> > -            (opcode >= FS_OPCODE_DDX_COARSE && opcode <=
> FS_OPCODE_LINTERP)));
> > +            (opcode >= FS_OPCODE_DDX_COARSE && opcode <=
> FS_OPCODE_LINTERP))) ||
> > +          (opcode == FS_OPCODE_LINTERP && !devinfo->has_pln);
>
> The devinfo->has_pln condition is technically inaccurate, because even
> SNB will fall back to the non-PLN path which overwrites the accumulator
> for certain valid IR, which yeah I'm aware is not *typically*
> encountered before this series,


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.


> but why make things more inaccurate than
> the original only to revert back to a devinfo->gen based check in
> PATCHv2 33?
>

See above.


> I think I'd squash the last hunk of PATCHv2 33 into this one which would
> give you something functionally equivalent to v1 but updated to handle
> Gen11 correctly.
>
> >  }
> >
> >  bool
> > --
> > 2.5.0.400.gff86faf
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180526/33a9e32a/attachment.html>