Re: [Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
On Fri, Jan 23, 2015 at 1:38 PM, Matt Turner wrote: > On Sat, Jan 17, 2015 at 12:07 AM, Kenneth Graunke > wrote: >> With an updated commit message and Piglit passing (I'll test and let you >> know), >> Reviewed-by: Kenneth Graunke > > Reminder to piglit these the next time you power on your Gen4. :) I've now run this on a gen4 and in debug builds its results are shaders/glsl-fs-sampler-numbering-2: crash pass shaders/glsl-fs-sampler-numbering-3: crash pass glslparsertest/glsl2/norsetto-bumptbn_sh_fp.frag: crash pass glslparsertest/glsl2/gst-gl-glass.frag: crash pass spec/glsl-1.10/execution/samplers/in-parameter-array: crash pass shaders/glsl-fs-texture2d-branching: crash pass No regressions. I'm planning to going to tag this for 10.5 and commit it. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
On Sat, Jan 17, 2015 at 12:07 AM, Kenneth Graunke wrote: > With an updated commit message and Piglit passing (I'll test and let you > know), > Reviewed-by: Kenneth Graunke Reminder to piglit these the next time you power on your Gen4. :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
On Friday, January 16, 2015 11:55:33 PM Matt Turner wrote:
> On Fri, Jan 16, 2015 at 11:45 PM, Kenneth Graunke
> wrote:
> > On Tuesday, January 13, 2015 03:35:57 PM Matt Turner wrote:
> >> This is a fix for a regression introduced in commit a9f8296d ("i965/fs:
> >> Preserve the CFG in a few more places.").
> >>
> >> The errata this code works around is described in a comment before the
> >> function:
> >>
> >>"[DevBW, DevCL] Errata: A destination register from a send can not be
> >> used as a destination register until after it has been sourced by an
> >> instruction with a different destination register.
> >>
> >> The framebuffer write's sources must be in message registers, which SEND
> >> instructions cannot have as a destination. There's no way for this
> >> errata to affect anything at the end of the program. Just remove the
> >> code.
> >
> > I don't think that's the point. The idea is that code such as
> >
> >SEND g10 ...sources... rlen 4
> >MUL g10 ... ...
> >
> > needs a workaround - you can't write to the destination of a SEND safely
> > without reading them first. You'd have to do:
> >
> >SEND g10 ...sources... rlen 4
> >MOV null g10 pointless read of g10, any instruction will do
> >MUL g10 ...
> >
> > Normally, the results of SEND instructions are actually used. However, they
> > aren't always - for example, depth texturing returns 4 values, but we only
> > care about the .X channel.
>
> Right, and we throw up our hands and resolve all remaining
> dependencies when we see the end of the basic block because there's a
> subsequent basic block that may write the destination.
>
> At the end of the program though... we can't possibly need to resolve
> anything outstanding because we can't possibly overwrite it. Can we?
I agree, I think this should be safe. It sounds like the effects of the bug
are an undefined write ordering...probably not GPU hangs. If that's true,
then we're obviously fine - we never overwrite it.
On the completely paranoid side of things, there could be some bit in the
hardware that leaves the register stuck: "I'm not done with the last write,
I need to stall until it completes before doing this one". And, it's
possible it could persist between threads. Which would leave us stalled
forever, and we'd hang the GPU.
But I sincerely doubt that's the case, and I agree with you that this should
be fine. I would like to see the commit message updated - instead of the bit
about MRFs, say that we think it's pointless to apply the workaround for
registers that are never written again, and that deleting the code is an
alternative to making it work in CFG-land.
With an updated commit message and Piglit passing (I'll test and let you know),
Reviewed-by: Kenneth Graunke
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
On Fri, Jan 16, 2015 at 11:45 PM, Kenneth Graunke wrote:
> On Tuesday, January 13, 2015 03:35:57 PM Matt Turner wrote:
>> This is a fix for a regression introduced in commit a9f8296d ("i965/fs:
>> Preserve the CFG in a few more places.").
>>
>> The errata this code works around is described in a comment before the
>> function:
>>
>>"[DevBW, DevCL] Errata: A destination register from a send can not be
>> used as a destination register until after it has been sourced by an
>> instruction with a different destination register.
>>
>> The framebuffer write's sources must be in message registers, which SEND
>> instructions cannot have as a destination. There's no way for this
>> errata to affect anything at the end of the program. Just remove the
>> code.
>
> I don't think that's the point. The idea is that code such as
>
>SEND g10 ...sources... rlen 4
>MUL g10 ... ...
>
> needs a workaround - you can't write to the destination of a SEND safely
> without reading them first. You'd have to do:
>
>SEND g10 ...sources... rlen 4
>MOV null g10 pointless read of g10, any instruction will do
>MUL g10 ...
>
> Normally, the results of SEND instructions are actually used. However, they
> aren't always - for example, depth texturing returns 4 values, but we only
> care about the .X channel.
Right, and we throw up our hands and resolve all remaining
dependencies when we see the end of the basic block because there's a
subsequent basic block that may write the destination.
At the end of the program though... we can't possibly need to resolve
anything outstanding because we can't possibly overwrite it. Can we?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
On Tuesday, January 13, 2015 03:35:57 PM Matt Turner wrote:
> This is a fix for a regression introduced in commit a9f8296d ("i965/fs:
> Preserve the CFG in a few more places.").
>
> The errata this code works around is described in a comment before the
> function:
>
>"[DevBW, DevCL] Errata: A destination register from a send can not be
> used as a destination register until after it has been sourced by an
> instruction with a different destination register.
>
> The framebuffer write's sources must be in message registers, which SEND
> instructions cannot have as a destination. There's no way for this
> errata to affect anything at the end of the program. Just remove the
> code.
I don't think that's the point. The idea is that code such as
SEND g10 ...sources... rlen 4
MUL g10 ... ...
needs a workaround - you can't write to the destination of a SEND safely
without reading them first. You'd have to do:
SEND g10 ...sources... rlen 4
MOV null g10 pointless read of g10, any instruction will do
MUL g10 ...
Normally, the results of SEND instructions are actually used. However, they
aren't always - for example, depth texturing returns 4 values, but we only
care about the .X channel.
In the past, we attempted to insert DEP_RESOLVE_MOVs to read every register
that was written by a SEND. With your patch, we will only insert them for
components that were actually used.
I imagine this will work fine - if you read the G45 PRM Volume 4 page 516,
you'll see all kinds of dependency tracking woes. Switching threads also
apparently solves the pre-send problem, so I imagine terminating the thread
without ever touching the register again would be safe.
I'm kind of tempted to keep the old behavior, out of paranoia, but I can
test it, at least.
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84613
> ---
> Untested on real hardware.
>
> src/mesa/drivers/dri/i965/brw_fs.cpp | 10 --
> 1 file changed, 10 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 9dfb7b7..3f9cd68 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2925,16 +2925,6 @@
> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
> fs_ins
>if (i == write_len)
> return;
> }
> -
> - /* If we hit the end of the program, resolve all remaining dependencies
> out
> -* of paranoia.
> -*/
> - fs_inst *last_inst = (fs_inst *)this->instructions.get_tail();
> - assert(last_inst->eot);
> - for (int i = 0; i < write_len; i++) {
> - if (needs_dep[i])
> - last_inst->insert_before(block, DEP_RESOLVE_MOV(first_write_grf +
> i));
> - }
> }
>
> void
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965/fs: Don't use backend_visitor::instructions after creating the CFG.
This is a fix for a regression introduced in commit a9f8296d ("i965/fs:
Preserve the CFG in a few more places.").
The errata this code works around is described in a comment before the function:
"[DevBW, DevCL] Errata: A destination register from a send can not be
used as a destination register until after it has been sourced by an
instruction with a different destination register.
The framebuffer write's sources must be in message registers, which SEND
instructions cannot have as a destination. There's no way for this
errata to affect anything at the end of the program. Just remove the
code.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84613
---
Untested on real hardware.
src/mesa/drivers/dri/i965/brw_fs.cpp | 10 --
1 file changed, 10 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 9dfb7b7..3f9cd68 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -2925,16 +2925,6 @@
fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block, fs_ins
if (i == write_len)
return;
}
-
- /* If we hit the end of the program, resolve all remaining dependencies out
-* of paranoia.
-*/
- fs_inst *last_inst = (fs_inst *)this->instructions.get_tail();
- assert(last_inst->eot);
- for (int i = 0; i < write_len; i++) {
- if (needs_dep[i])
- last_inst->insert_before(block, DEP_RESOLVE_MOV(first_write_grf + i));
- }
}
void
--
2.0.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
