Jason Ekstrand <ja...@jlekstrand.net> writes: > On Sat, May 26, 2018 at 12:21 PM, Francisco Jerez <curroje...@riseup.net> > wrote: > >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >> > On Sat, May 26, 2018 at 11:10 AM, Francisco Jerez <curroje...@riseup.net >> > >> > wrote: >> > >> >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >> >> >> > From: Francisco Jerez <curroje...@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-sta...@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. >> > >> >> I know the conditions for the non-PLN fall-back are not typically >> encountered on Gen5-6, but it's still valid IR, so this implementation >> of writes_accumulator_implicitly() relies on the behavior of the >> register allocator, the NIR-to-i965 translation pass and the rest of the >> visitor being exactly what you expect in every possible codepath for >> correctness. > > > It's already relied on for correctness because the LINE+MAC fallback that > we had before is wrong in SIMD16 (see patch 33). >
Yes, I had to do basically the same fix on my SIMD32 branch in order for it to pass piglit on Gen4-5. Still hardly an excuse to make more code rely on the same broken assumption which wasn't doing it before. > >> That wasn't the case in my original patch, nor in your >> series after PATCHv2 33 because this inaccuracy actually becomes a >> problem. Instead of introducing code which we know is dubiously >> correct, CC'ing mesa-stable, and then fixing it immediately, why don't >> we just do the obviously correct thing from the start? >> > > Sure, we can squash them together if you really want. But if you're > concerned about this restriction being valid in live code, then 33/53 also > needs to go to stable. I'm fine with that if you're unconvinced by my > argument that LINTERP with an odd coordinate never occurs. > > --Jason > > >> > >> >> 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 >> >> >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
