<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 17, 2014 at 10:22 PM, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes:<br>
<br>
> From: Connor Abbott <<a href="mailto:connor.abbott@intel.com" target="_blank">connor.abbott@intel.com</a>><br>
><br>
> This is similar to ir_print_visitor.cpp.<br>
<br>
<br>
</span><span>> +static void<br>
> +print_alu_src(nir_alu_src *src, FILE *fp)<br>
> +{<br>
> +   if (src->negate)<br>
> +      fprintf(fp, "-");<br>
> +   if (src->abs)<br>
> +      fprintf(fp, "abs(");<br>
> +<br>
> +   print_src(&src->src, fp);<br>
> +<br>
> +   if (src->swizzle[0] != 0 ||<br>
> +       src->swizzle[1] != 1 ||<br>
> +       src->swizzle[2] != 2 ||<br>
> +       src->swizzle[3] != 3) {<br>
> +      fprintf(fp, ".");<br>
> +      for (unsigned i = 0; i < 4; i++)<br>
> +         fprintf(fp, "%c", "xyzw"[src->swizzle[i]]);<br>
<br>
</span>Not necessary in the review process, but I've been thinking it would be<br>
nice to use nir_alu_instr_channel_used() to skip printing unused swizzle<br>
characters (replacing it with '_' maybe).  It would usually help me when<br>
I'm reading shader dumps.<br></blockquote><div><br></div><div>Yeah, that's a good plan.  Personally, I'd go for _ if it's an actual skip (due to a write-mask) and only printing 3 characters for, say, a vec3.  But yeah, that's another patch<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> +static void<br>
> +print_alu_instr(nir_alu_instr *instr, FILE *fp)<br>
> +{<br>
> +   if (instr->has_predicate) {<br>
> +      fprintf(fp, "(");<br>
> +      print_src(&instr->predicate, fp);<br>
> +      fprintf(fp, ") ");<br>
> +   }<br>
> +<br>
> +   print_alu_dest(&instr->dest, fp);<br>
> +<br>
> +   fprintf(fp, " = %s", nir_op_infos[instr->op].name);<br>
> +   if (instr->dest.saturate)<br>
> +      fprintf(fp, ".sat");<br>
> +   fprintf(fp, " ");<br>
> +<br>
> +   bool first = true;<br>
> +   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {<br>
> +      if (!first)<br>
> +         fprintf(fp, ", ");<br>
<br>
</span>replace !first with i != 0?<br>
<br>
(optional style change throughout the file)<br></blockquote><div><br></div><div>Yeah, that would be better.  I'll see how many times that shows up and I may make a squash-in patch for that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> +      print_alu_src(&instr->src[i], fp);<br>
> +<br>
> +      first = false;<br>
> +   }<br>
> +}<br>
<br>
<br>
> +<br>
</span><span>> +   bool offset_nonzero = false;<br>
> +   for (unsigned i = 0; i < 4; i++)<br>
> +      if (instr->const_offset[i] != 0) {<br>
> +         offset_nonzero = true;<br>
> +         break;<br>
> +      }<br>
> +<br>
> +   if (offset_nonzero) {<br>
> +      fprintf(fp, "[%i %i %i %i] (offset), ",<br>
> +              instr->const_offset[0], instr->const_offset[1],<br>
> +              instr->const_offset[2], instr->const_offset[3]);<br>
> +   }<br>
<br>
</span>cleanup: just replace the "offset_nonzero = true;" statement with this<br>
fprintf?  (also, {} around long multi-line blocks, please, even if<br>
they're a single statement).<br></blockquote><div><br></div><div>I fixed the {}, but elected to leave the other alone.  The loop checks for if any are non-zero and then the print uses all of them.  The other is correct and shorter, but I think this makes more sensel.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Other then these little cleanups,<br>
<br>
Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>><br>
<br>
I've got a patch in my series to make a single-instruction printer,<br>
which is really nice to have for debugging in various "unreachable"<br>
default blocks of switch statements and nir_validate and things.<br>
</blockquote></div></div></div>