On 12/15/2015 04:40 PM, Ilia Mirkin wrote: > Hardly a complete review, but a handful of comments: > > On Tue, Dec 15, 2015 at 6:05 PM, Miklós Máté <mtm...@gmail.com> wrote: >> --- >> src/mesa/Makefile.sources | 1 + >> src/mesa/state_tracker/st_atifs_to_tgsi.c | 798 >> ++++++++++++++++++++++++++++++ >> src/mesa/state_tracker/st_atifs_to_tgsi.h | 49 ++ >> src/mesa/state_tracker/st_atom_constbuf.c | 14 + >> src/mesa/state_tracker/st_cb_drawpixels.c | 1 + >> src/mesa/state_tracker/st_cb_program.c | 35 +- >> src/mesa/state_tracker/st_program.c | 22 + >> src/mesa/state_tracker/st_program.h | 1 + >> 8 files changed, 920 insertions(+), 1 deletion(-) >> create mode 100644 src/mesa/state_tracker/st_atifs_to_tgsi.c >> create mode 100644 src/mesa/state_tracker/st_atifs_to_tgsi.h >> >> +static struct ureg_src prepare_argument(struct st_translate *t, const >> unsigned argId, >> + const struct atifragshader_src_register *srcReg) >> +{ >> + struct ureg_src src = get_source(t, srcReg->Index); >> + struct ureg_dst arg = get_temp(t, MAX_NUM_FRAGMENT_REGISTERS_ATI+argId); >> + >> + switch (srcReg->argRep) { >> + case GL_NONE: >> + break; >> + case GL_RED: >> + src = ureg_swizzle(src, >> + TGSI_SWIZZLE_X, TGSI_SWIZZLE_X, TGSI_SWIZZLE_X, >> TGSI_SWIZZLE_X); >> + break; >> + case GL_GREEN: >> + src = ureg_swizzle(src, >> + TGSI_SWIZZLE_Y, TGSI_SWIZZLE_Y, TGSI_SWIZZLE_Y, >> TGSI_SWIZZLE_Y); >> + break; >> + case GL_BLUE: >> + src = ureg_swizzle(src, >> + TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z, >> TGSI_SWIZZLE_Z); >> + break; >> + case GL_ALPHA: >> + src = ureg_swizzle(src, >> + TGSI_SWIZZLE_W, TGSI_SWIZZLE_W, TGSI_SWIZZLE_W, >> TGSI_SWIZZLE_W); >> + break; >> + } >> + emit_insn(t, TGSI_OPCODE_MOV, &arg, 1, &src, 1); >> + >> + if (srcReg->argMod & GL_COMP_BIT_ATI) { >> + struct ureg_src modsrc[2]; >> + modsrc[0] = ureg_imm1f(t->ureg, 1.0); >> + modsrc[1] = ureg_src(arg); >> + >> + emit_insn(t, TGSI_OPCODE_SUB, &arg, 1, modsrc, 2); >> + } >> + if (srcReg->argMod & GL_BIAS_BIT_ATI) { >> + struct ureg_src modsrc[2]; >> + modsrc[0] = ureg_src(arg); >> + modsrc[1] = ureg_imm1f(t->ureg, 0.5); >> + >> + emit_insn(t, TGSI_OPCODE_SUB, &arg, 1, modsrc, 2); >> + } >> + if (srcReg->argMod & GL_2X_BIT_ATI) { >> + struct ureg_src modsrc[2]; >> + modsrc[0] = ureg_src(arg); >> + modsrc[1] = ureg_imm1f(t->ureg, 2.0); >> + >> + emit_insn(t, TGSI_OPCODE_MUL, &arg, 1, modsrc, 2); > > aka ADD arg, arg, arg > >> + } >> + if (srcReg->argMod & GL_NEGATE_BIT_ATI) { >> + struct ureg_src modsrc[2]; >> + modsrc[0] = ureg_src(arg); >> + modsrc[1] = ureg_imm1f(t->ureg, -1.0); >> + >> + emit_insn(t, TGSI_OPCODE_MUL, &arg, 1, modsrc, 2); > > aka NEG arg, arg > >> + } >> + return ureg_src(arg); >> +} >> + >> +/* These instructions have no direct equivalent in TGSI */ >> +static void emit_special_inst(struct st_translate *t, struct >> instruction_desc *desc, >> + struct ureg_dst *dst, struct ureg_src *args, unsigned argcount) >> +{ >> + struct ureg_dst tmp[1]; >> + struct ureg_src src[3]; >> + >> + if (desc->special == 1) { >> + tmp[0] = get_temp(t, MAX_NUM_FRAGMENT_REGISTERS_ATI+2); // re-purpose >> a3 >> + src[0] = ureg_imm1f(t->ureg, 0.5f); >> + src[1] = args[2]; >> + emit_insn(t, TGSI_OPCODE_SLT, tmp, 1, src, 2); >> + src[0] = ureg_src(tmp[0]); >> + src[1] = args[0]; >> + src[2] = args[1]; >> + emit_insn(t, TGSI_OPCODE_LRP, dst, 1, src, 3); >> + } else if (desc->special == 2) { >> + tmp[0] = get_temp(t, MAX_NUM_FRAGMENT_REGISTERS_ATI+2); // re-purpose >> a3 >> + src[0] = args[2]; >> + src[1] = ureg_imm1f(t->ureg, 0.0f); >> + emit_insn(t, TGSI_OPCODE_SGE, tmp, 1, src, 2); >> + src[0] = ureg_src(tmp[0]); >> + src[1] = args[0]; >> + src[2] = args[1]; >> + emit_insn(t, TGSI_OPCODE_LRP, dst, 1, src, 3); > > Isn't this the CMP instruction? Just flip the args. > > http://gallium.readthedocs.org/en/latest/tgsi.html#opcode-CMP > > The other one should be expressible as CMP as well I think. > >> + } else if (desc->special == 3) { >> + src[0] = args[0]; >> + src[1] = args[1]; >> + src[2] = ureg_swizzle(args[2], >> + TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z); >> + emit_insn(t, TGSI_OPCODE_DP2A, dst, 1, src, 3); >> + } >> +} >> + >> +static void emit_arith_inst(struct st_translate *t, >> + struct instruction_desc *desc, >> + struct ureg_dst *dst, struct ureg_src *args, unsigned argcount) >> +{ >> + if (desc->special) { >> + return emit_special_inst(t, desc, dst, args, argcount); >> + } >> + >> + emit_insn(t, desc->TGSI_opcode, dst, 1, args, argcount); >> +} >> + >> +static void emit_dstmod(struct st_translate *t, >> + struct ureg_dst dst, GLuint dstMod) >> +{ >> + float imm = 0.0; > > 1.0 right? (if you just have the saturate bit) > >> + struct ureg_src src[3]; >> + >> + if (dstMod == GL_NONE) { >> + return; >> + } >> + >> + if (dstMod & GL_2X_BIT_ATI) { >> + imm = 2.0f; >> + } else if (dstMod & GL_4X_BIT_ATI) { >> + imm = 4.0f; >> + } else if (dstMod & GL_8X_BIT_ATI) { >> + imm = 8.0f; >> + } else if (dstMod & GL_HALF_BIT_ATI) { >> + imm = 0.5f; >> + } else if (dstMod & GL_QUARTER_BIT_ATI) { >> + imm = 0.25f; >> + } else if (dstMod & GL_EIGHTH_BIT_ATI) { >> + imm = 0.125f; >> + } >> + if (imm) { > > and this should be unconditional > >> + src[0] = ureg_src(dst); >> + src[1] = ureg_imm1f(t->ureg, imm); >> + if (dstMod & GL_SATURATE_BIT_ATI) { >> + dst = ureg_saturate(dst); >> + } >> + emit_insn(t, TGSI_OPCODE_MUL, &dst, 1, src, 2); >> + } >> +} > > More generally, all this seems like pretty simple stuff (in terms of > instruction fanciness) -- would it have been better to try to convert > this to mesa ir instead, to enable this to be used on i965 as well, > which can consume mesa ir?
I was going to ask about going through NIR. We have some intention to get rid of Mesa eventually, and we already translate Mesa IR to NIR for i965. > Cheers, > > -ilia > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev