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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
