[Mesa-dev] [PATCH 07/17] intel/compiler/fs: Return multiple_instructions_emitted from generate_linterp
kenneth at whitecape.org
Sat Feb 24 00:08:03 UTC 2018
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
> 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...
Size: 833 bytes
Desc: This is a digitally signed message part.
More information about the mesa-dev