On Jul 29, 2015 3:12 AM, "Francisco Jerez" <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > On Tue, Jul 28, 2015 at 1:23 AM, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> 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(<a href="http://bld.at">bld.at</a>(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(<a href="http://bld.at">bld.at</a>(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(<a href="http://bld.at">bld.at</a>(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(<a href="http://bld.at">bld.at</a>(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 > >>