Jason Ekstrand <ja...@jlekstrand.net> writes: > On Mon, May 28, 2018 at 10:50 AM, Francisco Jerez <curroje...@riseup.net> > wrote: > >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >> > On Sat, May 26, 2018 at 6:33 PM, Francisco Jerez <curroje...@riseup.net> >> > wrote: >> > >> >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >> >> >> > On Sat, May 26, 2018 at 3:36 PM, Francisco Jerez < >> curroje...@riseup.net> >> >> > wrote: >> >> > >> >> >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >> >> >> >> >> > On Sat, May 26, 2018 at 2:03 PM, Francisco Jerez < >> >> curroje...@riseup.net> >> >> >> > wrote: >> >> >> > >> >> >> >> 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. >> >> >> > >> > I think this is the fundamental point of disagreement. What I would >> argue >> > (and what this series does) is that it's not possible to hit odd register >> > case and, if we do, so badly broken on g4x-SNB (uses the wrong register >> > layout in SIMD16 and doesn't actually bother to write the accumulator on >> > gen6) that we're better off if we just stop pretending it works. Patch >> > 1/53 of this series just removes the fall-back and replaces it with an >> > assert at which point the odd register fall-back path does not exist. >> > Jenkins is happy with us doing so. This patch then fixes >> > writes_accumulator_implicitly for the !has_pln case and the next patch >> > makes re-implements the the odd register case correctly because it really >> > is needed for SIMD32. >> > >> >> You're reiterating the same point. The content of the paragraph above >> was clear four e-mails ago. It still doesn't contain any convincing >> argument to me to split an obviously correct change into a buggy one >> followed by a bugfix. >> > > Would it make you happier if things were re-ordered such that we drop patch > 1, do the correct implementation of LINE+MAC first and then follow it with > a version of this patch that includes all the cases? >
I don't care about how you order the patches. > >> > If you're really concerned about the odd register case on the stable >> > branch, I'm happy to back port all three patches. It won't do any good >> but >> > it also won't cause any harm either. >> > >> >> I wasn't hugely concerned about the odd register case being hit on the >> stable branch (even though it passing Jenkins doesn't guarantee it will >> never be hit for some codepaths in the compiler which happen not to be >> hit by any test-case on the CI), I was only complaining about the >> seemingly idiotic structure of patches you had sent with my name. >> >> >>> > I'm very confused by what you mean here. The old code assumed the >> >> >> LINE+MAC >> >> >> > layout register layout which is different from the PLN layout. >> This >> >> >> means >> >> >> > that the fallback is broken for g45-gen6 because the hardware >> supports >> >> >> PLN >> >> >> > so we use the PLN layout which arranges thing x1y1x2y2 but the >> >> LINE+MAC >> >> >> > fallback assumes x1x2y1y2. The reason why we need so much more >> code >> >> is >> >> >> > because we have to split it all the way down to 8-wide in order to >> >> handle >> >> >> > the PLN layout with LINE+MAC. Looking at the code in your branch, >> I >> >> have >> >> >> > no idea how it could possibly work correctly. >> >> >> > >> >> >> >> >> >> It worked correctly with the LINE/MAC path (and in fact it passed >> piglit >> >> >> with SIMD32 enabled on Gen4-5) in exactly the same way PATCH 33 of >> this >> >> >> series does, but it used a loop instead of the 3x hand-unrolled code >> of >> >> >> PATCH 33, and it took advantage of the two accumulator registers for >> >> >> better pipelining. >> >> > >> >> > >> >> > I dug around a bit more in your your branch and I think I know what's >> >> going >> >> > on now. In your patch "i965/fs: Fix LINTERP instruction codegen for >> >> > misaligned regs on pre-Gen7 hw", you changed the LINE+MAC emit code to >> >> use >> >> > a loop to generate 1, 2, or 4 8-wide instructions. This implicitly >> >> > switches the assumed layout of barycentric coordinates in the >> fall-back >> >> > code to the PLN convention (x1y1x2y2) and breaks SIMD16 on gen4 which >> >> lays >> >> > the barycentric coordinates out as x1x2y1y2. In "i965/fs: Fix Gen4-5 >> >> > interpolation setup for SIMD32", you silently change the barycentric >> >> > coordinate layout convention on gen4 to be the PLN convention and it >> gets >> >> > fixed again. >> >> >> >> Yes, that's right, it relies on the other patch that makes the payload >> >> barycentric layout consistent across generations. And yes, I didn't >> >> spend any effort documenting the changes nor ordering things in a way >> >> that would allow bisection, since that branch wasn't meant to be >> >> reviewed nor merged upstream yet. >> >> >> > >> > Ok, at least we're now on the same page about how various branches work. >> > >> > >> >> > I'm also very confused about accumulators since the SNB documentation >> >> > says that we have 16 floats worth (two accumulators) and your branch >> >> > definitely does 4 8-wide LINE instructions followed by 4 8-wide MAC >> >> > instructions. I agree that taking advantage of the two accumulators >> >> > would be good, but the patch I see in your branch seems to be >> >> > incorrect. >> >> > >> >> No, my branch wasn't doing 4 8-wide instructions in a row, that would >> >> require four accumulators -- It was relying on SIMD lowering to split >> >> things up into 16-wide LINTERP instructions, just like your patch is. >> >> >> > >> > Ok, I see that now. Also, I just realized how creepy it is to have the >> > SIMD lowering pass lower LINTERP. It's not going to do it correctly >> > because it doesn't understand the channel layout of the payload when it's >> > set up for PLN. We get by because none of those MOVs ever happen in the >> > end. >> > >> >> Actually no, that was working fine. My branch addressed the problem by >> laying out SIMD32 payloads according to the inverse of the permutation >> that emit_unzip() will apply on it subsequently, like "x0y0 x2y2 x1y1 >> x3y3", which after unzip gives you "x0y0 x1y1" for the first half and >> "x2y2 x3y3" for the second. The MOVs should always be emitted by the >> SIMD lowering pass for those because they're multiple-component source >> regions, but they should be optimized out. >> > > Right. My comment there was that the MOVs will be emitted with the wrong > channel enables. We'll likely move x0y0 with one SIMD16 MOV and x1y1 with > another. Since the optimizer gets rid of the MOVs, it's not a problem. > You should probably fix that as part of this series. exec_all() is your friend. > >> > >> >> > All in all, changing the gen4 barycentric coordinate convention >> doesn't >> >> > seem like all that bad of a thing. It's a bit odd that we lay them >> out >> >> > differently on exactly one platform. I suppose, though, that we >> might be >> >> > able to generate SIMD16 MADs on gen11 if we started using the LINE >> >> > convention again. Not sure if it's worth it. >> >> > >> >> > >> >> >> I can send you the patch as a replacement for PATCH >> >> >> 33 if you care at all about performance of this corner case on >> Gen4-6... >> >> >> >> >> >> > My point is that the odd register fallback on g4x-gen6 was never >> used >> >> and >> >> >> > didn't work even if it was. >> >> >> > >> >> >> >> >> >> My point was that it doesn't matter for correctness of this patch >> >> >> whether another codepath in the compiler is buggy under approximately >> >> >> the same conditions. If you think there is some sort of benefit from >> >> >> committing knowingly broken code just so you can fix it in the next >> >> >> commit, feel free, but please drop my name from the author tag since >> I >> >> >> didn't intend it to. >> >> >> > >> > Done. >> > >> > >> >> >> > --Jason >> >> >> > >> >> >> > >> >> >> >> > >> >> >> >> >> 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