[Mesa-dev] [PATCH 07/17] intel/compiler/fs: Return multiple_instructions_emitted from generate_linterp

[Kenneth Graunke]

On Friday, February 23, 2018 3:51:37 PM PST Kenneth Graunke wrote:
> On Tuesday, February 20, 2018 9:15:14 PM PST Matt Turner wrote:
> > If multiple instructions are emitted, special handling of things like
> > conditional mod, saturate, and NoDDClr/NoDDChk need to be performed.
> > 
> > I noticed that conditional mods were misapplied when adding support for
> > Gen11 (in the previous patch). The next patch fixes the same bug in the
> > Gen4 LINE/MAC case, though I was not able to trigger it.
> > ---
> >  src/intel/compiler/brw_fs.h             |  2 +-
> >  src/intel/compiler/brw_fs_generator.cpp | 12 +++++++++---
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> Ugh...I noticed a couple things:
> 
> 1. Nothing has set multiple_instructions_emitted since May 2016
>    (commit 95272f5c7e6914fe8a85a4e37e07f1e8e3634446), which means
>    that block of code has been dead for a long time.
> 
> 2. Nothing in the FS backend sets inst->no_dd_* either.  It looks like
>    it was used for general interpolation until July 2016 (commit
>    1eef0b73aa323d94d5a080cd1efa81ccacdbd0d2) and for the unlit centroid
>    workaround until August 2016 (commit 875341c69b99dea7942a68c9060aa3).
>    So, that's also basically dead.
> 
> 3. You mention saturate, but we don't handle that specially for multiple
>    instructions.
> 
> 4. You didn't handle conditional modifiers in generate_linterp, so...
>    if conditional LINTERP is a thing, it's going to be broken.  That
>    said, I'm pretty sure it isn't a thing.

Sorry, this isn't quite true.  For Gen11+, you handle conditional mod
specially, because you need to put it on multiple instructions - one of
which is the last.  So, without multiple_instructions_emitted flagged,
your code would set it...and this code would set it again.  Harmless.

However, it looks like you haven't extended generate_linterp() to handle
conditional mod in the older LINE/MAC case.  So, with this patch, those
won't get conditions at all.  Without this patch, we would set it on the
final MAC...which I think is what we want.

So, I think this is unnecessary for Gen11+ and harmful for old HW.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180223/002d66f7/attachment.sig>