[Mesa-dev] [PATCH 03/11] st/mesa: implement GL_ATI_fragment_shader
Miklós Máté
mtmkls at gmail.com
Wed Dec 16 14:28:06 PST 2015
On 12/16/2015 01:40 AM, Ilia Mirkin wrote:
> Hardly a complete review, but a handful of comments:
Thank you for your comments. See my replies inline.
>
> On Tue, Dec 15, 2015 at 6:05 PM, Miklós Máté <mtmkls at 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
Fixed.
>
>> + }
>> + 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
I'd love to, but there is no such TGSI opcode.
>
>> + }
>> + 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
Yes, thanks. It was too far down, I didn't find it.
>
> The other one should be expressible as CMP as well I think.
Ok, I changed it to tmp=SUB(0.5, arg2); dst=CMP(tmp, arg0, arg1)
>
>> + } 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
Fixed. Initially I used TGSI_OPCODE_CLAMP below this block, but then I
discovered the saturate modifier, and the conditional somehow escaped my
attention.
>
>> + 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?
(reply to the whole thread on this)
I took a brief look on NIR and it seems much more complicated than TGSI.
I'd rather wait for the end of the IR-wars before taking action.
Meanwhile, the Intel GPUs can support this extension with ilo, or the
TGSI->NIR translator.
MM
More information about the mesa-dev
mailing list