<div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 14, 2019 at 1:18 PM Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Feb 11, 2019 at 8:44 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br>
><br>
> This fixes a bug in runscape where we were optimizing x >> 16 to an<br>
> extract and then negating and converting to float.  The NIR to fs pass<br>
> was dropping the negate on the floor breaking a geometry shader and<br>
> causing it to render nothing.<br>
><br>
> Fixes: 1f862e923cb "i965/fs: Optimize float conversions of byte/word..."<br>
> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=109601" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=109601</a><br>
> Cc: Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>><br>
> ---<br>
>  src/intel/compiler/brw_fs_nir.cpp | 5 +++++<br>
>  1 file changed, 5 insertions(+)<br>
><br>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp<br>
> index b80f4351b49..204640ac726 100644<br>
> --- a/src/intel/compiler/brw_fs_nir.cpp<br>
> +++ b/src/intel/compiler/brw_fs_nir.cpp<br>
> @@ -510,6 +510,11 @@ fs_visitor::optimize_extract_to_float(nir_alu_instr *instr,<br>
>         src0->op != nir_op_extract_i8 && src0->op != nir_op_extract_i16)<br>
>        return false;<br>
><br>
> +   /* If either opcode has source modifiers, bail. */<br>
> +   if (instr->src[0].abs || instr->src[0].negate ||<br>
> +        src0->src[0].abs || src0->src[0].negate)<br>
<br>
You've done something weird here to vertically align things. I don't<br>
know if I like it, but if you're doing to indent the 'src0' so that<br>
the rest aligns then at least be consistent and align both 'src0's :)<br></blockquote><div><br></div><div>That wasn't intentional.  I've changed it back to the conventional indentation so nothing lines up.<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">
> +      return false;<br>
> +<br>
<br>
We could handle this better by allowing the optimization if we're<br>
extracting the high byte/word. In that case I think we could safely<br>
apply the source mod. We should do that separately, but it might be<br>
nice to leave a FINISHME here so we'll remember. <br></blockquote><div><br></div><div>I dropped the following comment in:</div><div><br></div><div>+   /* If either opcode has source modifiers, bail.<br>+    *<br>+    * TODO: We can potentially handle source modifiers if both of the opcodes<br>+    * we're combining are signed integers.<br>+    */<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">
Reviewed-by: Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>><br></blockquote><div><br></div><div>Thanks! <br></div></div></div></div>