Re: [Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.
Jason Ekstrand writes:
> On Jul 29, 2015 3:12 AM, "Francisco Jerez" wrote:
>>
>> Jason Ekstrand writes:
>>
>> > On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez
> wrote:
>> >> Instead of relying on the default one. This shouldn't lead to any
>> >> functional changes because DEP_RESOLVE_MOV overrides the execution
>> >> controls of the instruction anyway.
>> >
>> > Actually, DEP_RESOLVE_MOV calls half() on the builder which doesn't
>> > make any sense. I think the pre-builder intention, based on the
>> > comment, was force_uncompressed and to use a SIMD8 instruction so that
>> > it adds as few deps as possible. I'm not sure what it's actually
>> > doing now. In any case, saying that it overrides everything isn't
>> > right.
>> >
>> Pre-builder it was doing the same it's doing now -- Emit an 8-wide
>> instruction regardless of the dispatch width of the shader.
>> fs_builder::half(0) is an alias for ::group(8, 0) which simply selects
>> the first 8-channel group of the channel enables and will cause the
>> instruction to be uncompressed during code generation.
>>
>> You're right that it doesn't override execution controls of the
>> instruction other than exec_size, but that's because it doesn't need to.
>> force_writemask_all and force_sechalf are fully irrelevant for what the
>> emitted MOV is intended to do: stall the pipeline until the instruction
>> that was writing the GRF provided as argument retires, which is going to
>> happen regardless of the EMask of the MOV instruction (even if it's zero
>> it should cause a stall because pre-IVB hardware didn't implement
>> shoot-down of instructions with zero EMask).
>
> Then let's change the commit message to say that the execution controls
> don't matter. With that, you can have my R-B on the last two.
>
Alright, I'll fix that.
> Is Tue SIMD stuff unblocked now?
Yeah, I already pushed most of it, thanks :)
> --Jason
>
>> >> ---
>> >> src/mesa/drivers/dri/i965/brw_fs.cpp | 11 +++
>> >> 1 file changed, 7 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> index 71d372c..8bc9372 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> @@ -2827,7 +2827,8 @@
> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>> >>if (block->start() == scan_inst) {
>> >> for (int i = 0; i < write_len; i++) {
>> >> if (needs_dep[i])
>> >> - DEP_RESOLVE_MOV(bld.at(block, inst), first_write_grf +
> i);
>> >> + DEP_RESOLVE_MOV(fs_builder(this, block, inst),
>> >> + first_write_grf + i);
>> >> }
>> >> return;
>> >>}
>> >> @@ -2843,7 +2844,7 @@
> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>> >> if (reg >= first_write_grf &&
>> >> reg < first_write_grf + write_len &&
>> >> needs_dep[reg - first_write_grf]) {
>> >> - DEP_RESOLVE_MOV(bld.at(block, inst), reg);
>> >> + DEP_RESOLVE_MOV(fs_builder(this, block, inst), reg);
>> >> needs_dep[reg - first_write_grf] = false;
>> >> if (scan_inst->exec_size == 16)
>> >>needs_dep[reg - first_write_grf + 1] = false;
>> >> @@ -2890,7 +2891,8 @@
> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
> fs_ins
>> >>if (block->end() == scan_inst) {
>> >> for (int i = 0; i < write_len; i++) {
>> >> if (needs_dep[i])
>> >> - DEP_RESOLVE_MOV(bld.at(block, scan_inst),
> first_write_grf + i);
>> >> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> >> + first_write_grf + i);
>> >> }
>> >> return;
>> >>}
>> >> @@ -2905,7 +2907,8 @@
> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
> fs_ins
>> >>scan_inst->dst.reg >= first_write_grf &&
>> >>scan_inst->dst.reg < first_write_grf + write_len &&
>> >>needs_dep[scan_inst->dst.reg - first_write_grf]) {
>> >> - DEP_RESOLVE_MOV(bld.at(block, scan_inst),
> scan_inst->dst.reg);
>> >> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> >> + scan_inst->dst.reg);
>> >> needs_dep[scan_inst->dst.reg - first_write_grf] = false;
>> >>}
>> >>
>> >> --
>> >> 2.4.6
>> >>
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.
On Jul 29, 2015 3:12 AM, "Francisco Jerez" wrote:
>
> Jason Ekstrand writes:
>
> > On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez
wrote:
> >> Instead of relying on the default one. This shouldn't lead to any
> >> functional changes because DEP_RESOLVE_MOV overrides the execution
> >> controls of the instruction anyway.
> >
> > Actually, DEP_RESOLVE_MOV calls half() on the builder which doesn't
> > make any sense. I think the pre-builder intention, based on the
> > comment, was force_uncompressed and to use a SIMD8 instruction so that
> > it adds as few deps as possible. I'm not sure what it's actually
> > doing now. In any case, saying that it overrides everything isn't
> > right.
> >
> Pre-builder it was doing the same it's doing now -- Emit an 8-wide
> instruction regardless of the dispatch width of the shader.
> fs_builder::half(0) is an alias for ::group(8, 0) which simply selects
> the first 8-channel group of the channel enables and will cause the
> instruction to be uncompressed during code generation.
>
> You're right that it doesn't override execution controls of the
> instruction other than exec_size, but that's because it doesn't need to.
> force_writemask_all and force_sechalf are fully irrelevant for what the
> emitted MOV is intended to do: stall the pipeline until the instruction
> that was writing the GRF provided as argument retires, which is going to
> happen regardless of the EMask of the MOV instruction (even if it's zero
> it should cause a stall because pre-IVB hardware didn't implement
> shoot-down of instructions with zero EMask).
Then let's change the commit message to say that the execution controls
don't matter. With that, you can have my R-B on the last two.
Is Tue SIMD stuff unblocked now?
--Jason
> >> ---
> >> src/mesa/drivers/dri/i965/brw_fs.cpp | 11 +++
> >> 1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index 71d372c..8bc9372 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -2827,7 +2827,8 @@
fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
> >>if (block->start() == scan_inst) {
> >> for (int i = 0; i < write_len; i++) {
> >> if (needs_dep[i])
> >> - DEP_RESOLVE_MOV(bld.at(block, inst), first_write_grf +
i);
> >> + DEP_RESOLVE_MOV(fs_builder(this, block, inst),
> >> + first_write_grf + i);
> >> }
> >> return;
> >>}
> >> @@ -2843,7 +2844,7 @@
fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
> >> if (reg >= first_write_grf &&
> >> reg < first_write_grf + write_len &&
> >> needs_dep[reg - first_write_grf]) {
> >> - DEP_RESOLVE_MOV(bld.at(block, inst), reg);
> >> + DEP_RESOLVE_MOV(fs_builder(this, block, inst), reg);
> >> needs_dep[reg - first_write_grf] = false;
> >> if (scan_inst->exec_size == 16)
> >>needs_dep[reg - first_write_grf + 1] = false;
> >> @@ -2890,7 +2891,8 @@
fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
fs_ins
> >>if (block->end() == scan_inst) {
> >> for (int i = 0; i < write_len; i++) {
> >> if (needs_dep[i])
> >> - DEP_RESOLVE_MOV(bld.at(block, scan_inst),
first_write_grf + i);
> >> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
> >> + first_write_grf + i);
> >> }
> >> return;
> >>}
> >> @@ -2905,7 +2907,8 @@
fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
fs_ins
> >>scan_inst->dst.reg >= first_write_grf &&
> >>scan_inst->dst.reg < first_write_grf + write_len &&
> >>needs_dep[scan_inst->dst.reg - first_write_grf]) {
> >> - DEP_RESOLVE_MOV(bld.at(block, scan_inst),
scan_inst->dst.reg);
> >> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
> >> + scan_inst->dst.reg);
> >> needs_dep[scan_inst->dst.reg - first_write_grf] = false;
> >>}
> >>
> >> --
> >> 2.4.6
> >>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.
Jason Ekstrand writes:
> On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez
> wrote:
>> Instead of relying on the default one. This shouldn't lead to any
>> functional changes because DEP_RESOLVE_MOV overrides the execution
>> controls of the instruction anyway.
>
> Actually, DEP_RESOLVE_MOV calls half() on the builder which doesn't
> make any sense. I think the pre-builder intention, based on the
> comment, was force_uncompressed and to use a SIMD8 instruction so that
> it adds as few deps as possible. I'm not sure what it's actually
> doing now. In any case, saying that it overrides everything isn't
> right.
>
Pre-builder it was doing the same it's doing now -- Emit an 8-wide
instruction regardless of the dispatch width of the shader.
fs_builder::half(0) is an alias for ::group(8, 0) which simply selects
the first 8-channel group of the channel enables and will cause the
instruction to be uncompressed during code generation.
You're right that it doesn't override execution controls of the
instruction other than exec_size, but that's because it doesn't need to.
force_writemask_all and force_sechalf are fully irrelevant for what the
emitted MOV is intended to do: stall the pipeline until the instruction
that was writing the GRF provided as argument retires, which is going to
happen regardless of the EMask of the MOV instruction (even if it's zero
it should cause a stall because pre-IVB hardware didn't implement
shoot-down of instructions with zero EMask).
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 11 +++
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 71d372c..8bc9372 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -2827,7 +2827,8 @@
>> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>>if (block->start() == scan_inst) {
>> for (int i = 0; i < write_len; i++) {
>> if (needs_dep[i])
>> - DEP_RESOLVE_MOV(bld.at(block, inst), first_write_grf + i);
>> + DEP_RESOLVE_MOV(fs_builder(this, block, inst),
>> + first_write_grf + i);
>> }
>> return;
>>}
>> @@ -2843,7 +2844,7 @@
>> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>> if (reg >= first_write_grf &&
>> reg < first_write_grf + write_len &&
>> needs_dep[reg - first_write_grf]) {
>> - DEP_RESOLVE_MOV(bld.at(block, inst), reg);
>> + DEP_RESOLVE_MOV(fs_builder(this, block, inst), reg);
>> needs_dep[reg - first_write_grf] = false;
>> if (scan_inst->exec_size == 16)
>>needs_dep[reg - first_write_grf + 1] = false;
>> @@ -2890,7 +2891,8 @@
>> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
>> fs_ins
>>if (block->end() == scan_inst) {
>> for (int i = 0; i < write_len; i++) {
>> if (needs_dep[i])
>> - DEP_RESOLVE_MOV(bld.at(block, scan_inst), first_write_grf +
>> i);
>> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> + first_write_grf + i);
>> }
>> return;
>>}
>> @@ -2905,7 +2907,8 @@
>> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
>> fs_ins
>>scan_inst->dst.reg >= first_write_grf &&
>>scan_inst->dst.reg < first_write_grf + write_len &&
>>needs_dep[scan_inst->dst.reg - first_write_grf]) {
>> - DEP_RESOLVE_MOV(bld.at(block, scan_inst), scan_inst->dst.reg);
>> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
>> + scan_inst->dst.reg);
>> needs_dep[scan_inst->dst.reg - first_write_grf] = false;
>>}
>>
>> --
>> 2.4.6
>>
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.
On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez wrote:
> Instead of relying on the default one. This shouldn't lead to any
> functional changes because DEP_RESOLVE_MOV overrides the execution
> controls of the instruction anyway.
Actually, DEP_RESOLVE_MOV calls half() on the builder which doesn't
make any sense. I think the pre-builder intention, based on the
comment, was force_uncompressed and to use a SIMD8 instruction so that
it adds as few deps as possible. I'm not sure what it's actually
doing now. In any case, saying that it overrides everything isn't
right.
> ---
> src/mesa/drivers/dri/i965/brw_fs.cpp | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 71d372c..8bc9372 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2827,7 +2827,8 @@
> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
>if (block->start() == scan_inst) {
> for (int i = 0; i < write_len; i++) {
> if (needs_dep[i])
> - DEP_RESOLVE_MOV(bld.at(block, inst), first_write_grf + i);
> + DEP_RESOLVE_MOV(fs_builder(this, block, inst),
> + first_write_grf + i);
> }
> return;
>}
> @@ -2843,7 +2844,7 @@
> fs_visitor::insert_gen4_pre_send_dependency_workarounds(bblock_t *block,
> if (reg >= first_write_grf &&
> reg < first_write_grf + write_len &&
> needs_dep[reg - first_write_grf]) {
> - DEP_RESOLVE_MOV(bld.at(block, inst), reg);
> + DEP_RESOLVE_MOV(fs_builder(this, block, inst), reg);
> needs_dep[reg - first_write_grf] = false;
> if (scan_inst->exec_size == 16)
>needs_dep[reg - first_write_grf + 1] = false;
> @@ -2890,7 +2891,8 @@
> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
> fs_ins
>if (block->end() == scan_inst) {
> for (int i = 0; i < write_len; i++) {
> if (needs_dep[i])
> - DEP_RESOLVE_MOV(bld.at(block, scan_inst), first_write_grf +
> i);
> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
> + first_write_grf + i);
> }
> return;
>}
> @@ -2905,7 +2907,8 @@
> fs_visitor::insert_gen4_post_send_dependency_workarounds(bblock_t *block,
> fs_ins
>scan_inst->dst.reg >= first_write_grf &&
>scan_inst->dst.reg < first_write_grf + write_len &&
>needs_dep[scan_inst->dst.reg - first_write_grf]) {
> - DEP_RESOLVE_MOV(bld.at(block, scan_inst), scan_inst->dst.reg);
> + DEP_RESOLVE_MOV(fs_builder(this, block, scan_inst),
> + scan_inst->dst.reg);
> needs_dep[scan_inst->dst.reg - first_write_grf] = false;
>}
>
> --
> 2.4.6
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
