[Mesa-dev] [PATCH 006/133] nir: add a printer

[Connor Abbott]

On Thu, Dec 18, 2014 at 5:01 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Wed, Dec 17, 2014 at 10:22 PM, Eric Anholt <eric at anholt.net> wrote:
>>
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>> > From: Connor Abbott <connor.abbott at intel.com>
>> >
>> > This is similar to ir_print_visitor.cpp.
>>
>>
>> > +static void
>> > +print_alu_src(nir_alu_src *src, FILE *fp)
>> > +{
>> > +   if (src->negate)
>> > +      fprintf(fp, "-");
>> > +   if (src->abs)
>> > +      fprintf(fp, "abs(");
>> > +
>> > +   print_src(&src->src, fp);
>> > +
>> > +   if (src->swizzle[0] != 0 ||
>> > +       src->swizzle[1] != 1 ||
>> > +       src->swizzle[2] != 2 ||
>> > +       src->swizzle[3] != 3) {
>> > +      fprintf(fp, ".");
>> > +      for (unsigned i = 0; i < 4; i++)
>> > +         fprintf(fp, "%c", "xyzw"[src->swizzle[i]]);
>>
>> Not necessary in the review process, but I've been thinking it would be
>> nice to use nir_alu_instr_channel_used() to skip printing unused swizzle
>> characters (replacing it with '_' maybe).  It would usually help me when
>> I'm reading shader dumps.
>
>
> 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

Yeah, that makes a lot of sense.

>
>>
>>
>> > +static void
>> > +print_alu_instr(nir_alu_instr *instr, FILE *fp)
>> > +{
>> > +   if (instr->has_predicate) {
>> > +      fprintf(fp, "(");
>> > +      print_src(&instr->predicate, fp);
>> > +      fprintf(fp, ") ");
>> > +   }
>> > +
>> > +   print_alu_dest(&instr->dest, fp);
>> > +
>> > +   fprintf(fp, " = %s", nir_op_infos[instr->op].name);
>> > +   if (instr->dest.saturate)
>> > +      fprintf(fp, ".sat");
>> > +   fprintf(fp, " ");
>> > +
>> > +   bool first = true;
>> > +   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>> > +      if (!first)
>> > +         fprintf(fp, ", ");
>>
>> replace !first with i != 0?
>>
>> (optional style change throughout the file)
>
>
> Yeah, that would be better.  I'll see how many times that shows up and I may
> make a squash-in patch for that.
>
>>
>>
>> > +      print_alu_src(&instr->src[i], fp);
>> > +
>> > +      first = false;
>> > +   }
>> > +}
>>
>>
>> > +
>> > +   bool offset_nonzero = false;
>> > +   for (unsigned i = 0; i < 4; i++)
>> > +      if (instr->const_offset[i] != 0) {
>> > +         offset_nonzero = true;
>> > +         break;
>> > +      }
>> > +
>> > +   if (offset_nonzero) {
>> > +      fprintf(fp, "[%i %i %i %i] (offset), ",
>> > +              instr->const_offset[0], instr->const_offset[1],
>> > +              instr->const_offset[2], instr->const_offset[3]);
>> > +   }
>>
>> cleanup: just replace the "offset_nonzero = true;" statement with this
>> fprintf?  (also, {} around long multi-line blocks, please, even if
>> they're a single statement).
>
>
> 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.
>
>>
>>
>> Other then these little cleanups,
>>
>> Reviewed-by: Eric Anholt <eric at anholt.net>
>>
>> I've got a patch in my series to make a single-instruction printer,
>> which is really nice to have for debugging in various "unreachable"
>> default blocks of switch statements and nir_validate and things.
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>