<div dir="auto"> <div class="gmail_extra" dir="auto">Hi Alejandro,</div><div class="gmail_extra" dir="auto"><div dir="auto" style="font-family:sans-serif"> </div><div dir="auto" style="font-family:sans-serif">This seems really suspicious. If the live ranges are really independent, then the register allocator should be able to assign the two virtual registers to the same physical register if it needs to. This change forces the two to be the same, which constrains the register allocator unecessarily and should make it worse, so I'm confused as to why this would help at all.</div><div dir="auto" style="font-family:sans-serif"> </div><div dir="auto" style="font-family:sans-serif">IIRC there were some issues where we unnecessarily made the sources and destination of an instruction interefere with each other, but if that's what's causing this, then we should fix that underlying issue.</div><div dir="auto" style="font-family:sans-serif"> </div><div dir="auto" style="font-family:sans-serif">(From what I remember, a lot of SIMD16 were expanded to SIMD8 in the generator, in which case the second half of the source is read after the first half of the destination is written, and we falsely thought that the HW did that too, so we had some code to add a fake interference between them, but a while ago Curro moved the expansion to happen before register allocation. I don't have the code in front of me, but I think we still have this useless code lying around, and I would guess this is the source of the problem.)</div><div dir="auto" style="font-family:sans-serif"> </div><div dir="auto" style="font-family:sans-serif">Connor</div> <div class="gmail_quote" dir="auto">On Aug 24, 2017 2:59 PM, "Alejandro Piñeiro" <<a href="mailto:apinheiro@igalia.com" target="_blank">apinheiro@igalia.com</a>> wrote: <blockquote class="m_2012952128143901494quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When dealing with HF/U/UW, it is usual having a register with a F/D/UD, and then convert it to HF/U/UW, and not use again the F/D/UD value. In those cases it would be possible to reuse the register where the F value is initially stored instead of having two. Take also into account that when operating with HF/U/UW, you would need to use the full register (so stride 2). Packs/unpacks would be only useful when loading/storing several HF/W/UW. Note that no instruction is removed. The main benefict is reducing the amoung of registers used, so the pressure on the register allocator is decreased with big shaders. Possibly this could be integrated into an existing optimization, at it is even already done by the register allocator, but was far easier to write and cleaner to read as a separate optimization. We found this issue when dealing with some Vulkan CTS tests that needed several minutes to compile. Most of the time was spent on the register allocator. Right now the optimization only handles 32 to 16 bit conversion. It could be possible to do the equivalent for 16 to 32 bit too, but in practice, we didn't need it. --- src/intel/compiler/brw_fs.cpp | 77 +++++++++++++++++++++++++++++++++++++++++++ src/intel/compiler/brw_fs.h | 1 + 2 files changed, 78 insertions(+) diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp index b6013a5ce85..1342150b44e 100644 --- a/src/intel/compiler/brw_fs.cpp +++ b/src/intel/compiler/brw_fs.cpp @@ -39,6 +39,7 @@ #include "compiler/glsl_types.h" #include "compiler/nir/nir_builder.h" #include "program/prog_parameter.h" +#include "brw_fs_live_variables.h" using namespace brw; @@ -3133,6 +3134,81 @@ fs_visitor::remove_extra_rounding_modes() return progress; } +/** + * When dealing with HF/W/UW, it is usual having a register with a F/D/UD, and + * then convert it to HF/W/UW, and not use again the F/D/UD value. In those + * cases it would be possible to reuse the register where the F value is + * initially stored instead of having two. Take also into account that when + * operating with HF/W/UW, you would need to use the full register (so stride + * 2). Packs/unpacks would be only useful when loading/storing several + * HF/W/UWs. + * + * So something like this: + * mov(8) vgrf14<2>:HF, vgrf39:F + * + * Became: + * mov(8) vgrf39<2>:HF, vgrf39:F + * + * Note that no instruction is removed. The main benefict is reducing the + * amoung of registers used, so the pressure on the register allocator is + * decreased with big shaders. + */ +bool +fs_visitor::reuse_16bit_conversions_vgrf() +{ + bool progress = false; + int ip = -1; + + calculate_live_intervals(); + + foreach_block_and_inst_safe (block, fs_inst, inst, cfg) { + ip++; + + if (inst->dst.file != VGRF || inst->src[0].file != VGRF) + continue; + + if (inst->opcode != BRW_OPCODE_MOV) + continue; + + if (type_sz(inst->dst.type) != 2 || inst->dst.stride != 2 || + type_sz(inst->src[0].type) != 4 || inst->src[0].stride != 1) { + continue; + } + + int src_reg = inst->src[0].nr; + int src_offset = inst->src[0].offset; + unsigned src_var = live_intervals->var_from_vgrf[src_reg]; + int src_end = live_intervals->end[src_var]; + int dst_reg = inst-><a href="http://dst.nr" rel="noreferrer" target="_blank">dst.nr</a>; + + if (src_end > ip) + continue; + + foreach_block_and_inst(block, fs_inst, scan_inst, cfg) { + if (scan_inst->dst.file == VGRF && + scan_inst-><a href="http://dst.nr" rel="noreferrer" target="_blank">dst.nr</a> == dst_reg) { + scan_inst-><a href="http://dst.nr" rel="noreferrer" target="_blank">dst.nr</a> = src_reg; + scan_inst->dst.offset = src_offset; + progress = true; + } + + for (int i = 0; i < scan_inst->sources; i++) { + if (scan_inst->src[i].file == VGRF && + scan_inst->src[i].nr == dst_reg) { + scan_inst->src[i].nr = src_reg; + scan_inst->src[i].offset = src_offset; + progress = true; + } + } + } + } + + if (progress) + invalidate_live_intervals(); + + return progress; +} + static void clear_deps_for_inst_src(fs_inst *inst, bool *deps, int first_grf, int grf_len) @@ -5829,6 +5905,7 @@ fs_visitor::optimize() OPT(opt_drop_redundant_mov_to_flags); OPT(remove_extra_rounding_modes); + OPT(reuse_16bit_conversions_vgrf); do { progress = false; diff --git a/src/intel/compiler/brw_fs.h b/src/intel/compiler/brw_fs.h index b9476e69edb..2685f748b87 100644 --- a/src/intel/compiler/brw_fs.h +++ b/src/intel/compiler/brw_fs.h @@ -151,6 +151,7 @@ public: bool dead_code_eliminate(); bool remove_duplicate_mrf_writes(); bool remove_extra_rounding_modes(); + bool reuse_16bit_conversions_vgrf(); bool opt_sampler_eot(); bool virtual_grf_interferes(int a, int b); -- 2.11.0 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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> </blockquote></div> </div></div>