On Wed, Dec 17, 2014 at 10:22 PM, Eric Anholt <e...@anholt.net> wrote:
>
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
> > From: Connor Abbott <connor.abb...@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
>
> > +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 <e...@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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
