<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Sat, May 28, 2016 at 1:21 PM, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > On Fri, May 27, 2016 at 7:05 PM, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> > wrote: > >> This makes the whole LOAD_PAYLOAD munging unnecessary which simplifies >> the code and will allow the optimization to succeed in more cases >> independent of whether the LOAD_PAYLOAD instruction can be found or >> not. >> --- >> src/mesa/drivers/dri/i965/brw_fs.cpp | 79 >> +++++++++++------------------------- >> 1 file changed, 23 insertions(+), 56 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp >> b/src/mesa/drivers/dri/i965/brw_fs.cpp >> index 45c4753..6abf776 100644 >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp >> @@ -2612,11 +2612,15 @@ fs_visitor::opt_sampler_eot() >> if (key->nr_color_regions != 1) >> return false; >> >> + /* Requires clamping the color payload in the shader. */ >> + if (key->clamp_fragment_color) >> + return false; >> > > This looks new... Is it actually needed? Could we simply do "if > (tex_inst->saturate) return false;"? In any case, it should probably be > it's own commit. > </div></div>It is necessary to preserve the current behavior and skip the optimization when fragment color clamping is required, because right now the MOVs used to saturate the color values are inserted during logical send lowering so this pass would have no other way to find out. I can make the change in a separate commit just before this one if you like. </blockquote><div> </div><div>Right. No need to make it a separate commit. Just update the comment so it says something about the MOVs inserted during lowering. </div><div>Incidentally, I think Rob Clark recently added some stuff to do that lowering in NIR... In the future, we might want to use that and delete more back-end code. For now, an udpated comment will do. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > >> + >> /* Look for a texturing instruction immediately before the final >> FB_WRITE. */ >> bblock_t *block = cfg->blocks[cfg->num_blocks - 1]; >> fs_inst *fb_write = (fs_inst *)block->end(); >> assert(fb_write->eot); >> - assert(fb_write->opcode == FS_OPCODE_FB_WRITE); >> + assert(fb_write->opcode == FS_OPCODE_FB_WRITE_LOGICAL); >> >> /* There wasn't one; nothing to do. */ >> if (unlikely(fb_write->prev->is_head_sentinel())) >> @@ -2629,24 +2633,20 @@ fs_visitor::opt_sampler_eot() >> * “Response Length of zero is allowed on all SIMD8* and SIMD16* >> sampler >> * messages except sample+killpix, resinfo, sampleinfo, LOD, and >> gather4*” >> */ >> - if (!tex_inst->is_tex() || >> - tex_inst->opcode == SHADER_OPCODE_TXS || >> - tex_inst->opcode == SHADER_OPCODE_SAMPLEINFO || >> - tex_inst->opcode == SHADER_OPCODE_LOD || >> - tex_inst->opcode == SHADER_OPCODE_TG4 || >> - tex_inst->opcode == SHADER_OPCODE_TG4_OFFSET) >> + if (tex_inst->opcode != SHADER_OPCODE_TEX_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXD_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXF_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXL_LOGICAL && >> + tex_inst->opcode != FS_OPCODE_TXB_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXF_CMS_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXF_CMS_W_LOGICAL && >> + tex_inst->opcode != SHADER_OPCODE_TXF_UMS_LOGICAL) >> return false; >> > > Same here. This isn't just a rename, it's a switch from listing invalid > cases to explicitly listing valid ones. > </div></div>The reason is that is_tex() is unaware of the logical messages, I considered changing that but a few places expect instructions marked is_tex to be send-like (e.g. fs_inst::is_send_from_grf, fs_inst::regs_read), so we would have to stop using is_tex() in those places if we change it. </blockquote><div> </div><div>Right. Makes sense. Seems fine to me. Feel free to leave this patch as-is apart from a better comment above. </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > Other than those two comments, I really like this. It's a nice cleanup. > > >> >> - /* If there's no header present, we need to munge the LOAD_PAYLOAD as >> well. >> - * It's very likely to be the previous instruction. >> - */ >> + /* XXX - This shouldn't be necessary. */ >> if (tex_inst->prev->is_head_sentinel()) >> return false; >> >> - fs_inst *load_payload = (fs_inst *) tex_inst->prev; >> - if (load_payload->opcode != SHADER_OPCODE_LOAD_PAYLOAD) >> - return false; >> - >> assert(!tex_inst->eot); /* We can't get here twice */ >> assert((tex_inst->offset & (0xff << 24)) == 0); >> >> @@ -2658,46 +2658,10 @@ fs_visitor::opt_sampler_eot() >> tex_inst->regs_written = 0; >> fb_write->remove(cfg->blocks[cfg->num_blocks - 1]); >> >> - /* If a header is present, marking the eot is sufficient. Otherwise, >> we need >> - * to create a new LOAD_PAYLOAD command with the same sources and a >> space >> - * saved for the header. Using a new destination register not only >> makes sure >> - * we have enough space, but it will make sure the dead code >> eliminator kills >> - * the instruction that this will replace. >> + /* Marking EOT is sufficient, lower_logical_sends() will notice the EOT >> + * flag and submit a header together with the sampler message as >> required >> + * by the hardware. >> */ >> - if (tex_inst->header_size != 0) { >> - invalidate_live_intervals(); >> - return true; >> - } >> - >> - fs_reg send_header = ibld.vgrf(BRW_REGISTER_TYPE_F, >> - load_payload->sources + 1); >> - fs_reg *new_sources = >> - ralloc_array(mem_ctx, fs_reg, load_payload->sources + 1); >> - >> - new_sources[0] = fs_reg(); >> - for (int i = 0; i < load_payload->sources; i++) >> - new_sources[i+1] = load_payload->src[i]; >> - >> - /* The LOAD_PAYLOAD helper seems like the obvious choice here. >> However, it >> - * requires a lot of information about the sources to appropriately >> figure >> - * out the number of registers needed to be used. Given this stage in >> our >> - * optimization, we may not have the appropriate GRFs required by >> - * LOAD_PAYLOAD at this point (copy propagation). Therefore, we need to >> - * manually emit the instruction. >> - */ >> - fs_inst *new_load_payload = new(mem_ctx) >> fs_inst(SHADER_OPCODE_LOAD_PAYLOAD, >> - >> load_payload->exec_size, >> - send_header, >> - new_sources, >> - load_payload->sources >> + 1); >> - >> - new_load_payload->regs_written = load_payload->regs_written + 1; >> - new_load_payload->header_size = 1; >> - tex_inst->mlen++; >> - tex_inst->header_size = 1; >> - tex_inst->insert_before(cfg->blocks[cfg->num_blocks - 1], >> new_load_payload); >> - tex_inst->src[0] = send_header; >> - >> invalidate_live_intervals(); >> return true; >> } >> @@ -4153,7 +4117,7 @@ lower_sampler_logical_send_gen7(const fs_builder >> &bld, fs_inst *inst, opcode op, >> sources[i] = bld.vgrf(BRW_REGISTER_TYPE_F); >> >> if (op == SHADER_OPCODE_TG4 || op == SHADER_OPCODE_TG4_OFFSET || >> - offset_value.file != BAD_FILE || >> + offset_value.file != BAD_FILE || inst->eot || >> op == SHADER_OPCODE_SAMPLEINFO || >> is_high_sampler(devinfo, sampler)) { >> /* For general texture offsets (no txf workaround), we need a >> header to >> @@ -4174,7 +4138,7 @@ lower_sampler_logical_send_gen7(const fs_builder >> &bld, fs_inst *inst, opcode op, >> * and we have an explicit header, we need to set up the sampler >> * writemask. It's reversed from normal: 1 means "don't write". >> */ >> - if (inst->regs_written != 4 * reg_width) { >> + if (!inst->eot && inst->regs_written != 4 * reg_width) { >> assert((inst->regs_written % reg_width) == 0); >> unsigned mask = ~((1 << (inst->regs_written / reg_width)) - 1) & >> 0xf; >> inst->offset |= mask << 12; >> @@ -5738,6 +5702,10 @@ fs_visitor::optimize() >> pass_num = 0; >> >> OPT(lower_simd_width); >> + >> + /* After SIMD lowering just in case we had to unroll the EOT send. */ >> + OPT(opt_sampler_eot); >> + >> OPT(lower_logical_sends); >> >> if (progress) { >> @@ -5761,7 +5729,6 @@ fs_visitor::optimize() >> } >> >> OPT(opt_redundant_discard_jumps); >> - OPT(opt_sampler_eot); >> >> if (OPT(lower_load_payload)) { >> split_virtual_grfs(); >> -- >> 2.7.3 >> >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> </div></div></blockquote></div> </div></div>