<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 27, 2017 at 4:47 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:<br>
> The automatic exec size inference can accidentally mess things up if<br>
> we're not careful. For instance, if we have<br>
><br>
> add(4) g38.2<4>D g38.1<<wbr>8,2,4>D g38.2<8,2,4>D<br>
><br>
> then the destination register will end up having a width of 2 with a<br>
> horizontal stride of 4 and a vertical stride of 8. The EU emit code<br>
> sees the width of 2 and decides that we really wanted an exec size of<br>
> 2<br>
> which doesn't do what we wanted.<br>
<br>
Right :-/<br>
<br>
I have to say that this change makes me a little nervous, mostly<br>
because it doesn't look easy to identify all the cases where we were<br>
relying on automatic execsizes to fix things up for us... since this is<br>
not as easy as to look for places where we use 'vec1' or something like<br>
that. How did you get the list of things that needed explicit sizes?<br></blockquote><div><br></div><div>Lots of grep :) If I recall correctly, I searched for EXECUTE_1, vec1, EXECUTE_2, and vec2.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, both commits before this address cases of exec_size = 1, but we<br>
rely on automatic exec sizes for exec_size = 2 as well, I guess we have<br>
none of these?<br>
<br>
Anyway, I guess Jenkins would have caught at least most omissions so<br>
maybe I am being too paranoid.<span class="gmail-im"></span></blockquote><div><br></div><div>That's my hope as well. :-) Durring the debugging of this chunk of the series, Jenkins found quite a few errors. Once Jenkins was happy, I did one more pass of grep to be sure.</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ---<br>
> src/intel/compiler/brw_fs_<wbr>generator.cpp | 12 +++++++++---<br>
> 1 file changed, 9 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> b/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> index 8322be1..46f9a33 100644<br>
> --- a/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> +++ b/src/intel/compiler/brw_fs_<wbr>generator.cpp<br>
> @@ -190,6 +190,12 @@ fs_generator::fs_generator(<wbr>const struct<br>
> brw_compiler *compiler, void *log_data,<br>
> {<br>
> p = rzalloc(mem_ctx, struct brw_codegen);<br>
> brw_init_codegen(devinfo, p, mem_ctx);<br>
> +<br>
> + /* In the FS code generator, we are very careful to ensure that<br>
> we always<br>
> + * set the right execution size so we don't need the EU code to<br>
> "help" us<br>
> + * by trying to infer it. Sometimes, it infers the wrong thing.<br>
> + */<br>
> + p->automatic_exec_sizes = false;<br>
> }<br>
> <br>
> fs_generator::~fs_generator()<br>
> @@ -395,17 +401,17 @@ fs_generator::generate_fb_<wbr>write(fs_inst *inst,<br>
> struct brw_reg payload)<br>
> struct brw_reg v1_null_ud = vec1(retype(brw_null_reg(),<br>
> BRW_REGISTER_TYPE_UD));<br>
> <br>
> /* Check runtime bit to detect if we have to send AA data or<br>
> not */<br>
> - brw_set_default_<wbr>compression_control(p, BRW_COMPRESSION_NONE);<br>
> brw_push_insn_state(p);<br>
> - brw_inst_set_exec_size(<wbr>p->devinfo, brw_last_inst,<br>
> BRW_EXECUTE_1);<br>
> + brw_set_default_<wbr>compression_control(p, BRW_COMPRESSION_NONE);<br>
> + brw_set_default_exec_<wbr>size(p, BRW_EXECUTE_1);<br>
> brw_AND(p,<br>
> v1_null_ud,<br>
> retype(brw_<wbr>vec1_grf(1, 6), BRW_REGISTER_TYPE_UD),<br>
> brw_imm_ud(1<<<wbr>26));<br>
> brw_inst_set_cond_<wbr>modifier(p->devinfo, brw_last_inst,<br>
> BRW_CONDITIONAL_NZ);<br>
> - brw_pop_insn_state(p);<br>
> <br>
> int jmp = brw_JMPI(p, brw_imm_ud(0), BRW_PREDICATE_NORMAL) -<br>
> p->store;<br>
> + brw_pop_insn_state(p);<br>
> {<br>
> /* Don't send AA data */<br>
> fire_fb_write(inst, offset(payload, 1), implied_header,<br>
> inst->mlen-1);<br>
</blockquote></div><br></div></div>