Re: [Mesa-dev] [PATCH 08/14] i965/fs: Initialize a builder explicitly in the gen4 send dependency work-arounds.
Jason Ekstrand ja...@jlekstrand.net writes:
On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez curroje...@riseup.net
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 Jul 29, 2015 3:12 AM, Francisco Jerez curroje...@riseup.net wrote:
Jason Ekstrand ja...@jlekstrand.net writes:
On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez curroje...@riseup.net
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 ja...@jlekstrand.net writes:
On Jul 29, 2015 3:12 AM, Francisco Jerez curroje...@riseup.net wrote:
Jason Ekstrand ja...@jlekstrand.net writes:
On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez curroje...@riseup.net
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 Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez curroje...@riseup.net 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
