[Mesa-dev] [PATCH 03/11] st/mesa: implement GL_ATI_fragment_shader
Ian Romanick
idr at freedesktop.org
Tue Dec 15 17:58:52 PST 2015
On 12/15/2015 05:08 PM, Ilia Mirkin wrote:
> On Tue, Dec 15, 2015 at 7:59 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> 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é <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
>>>
>>>> + }
>>>> + 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.
>
> Then there would be 2 separate converters, which seems undesirable. I
> was suggesting using mesa IR to reduce that burden.
But we can already do NIR -> TGSI, right? So isn't it still just one
converter? It also seems undesirable to add a bunch of code that will
just need to get rewritten in the near future. Since I've already
signed up to do a bunch of the Mesa IR removal, I'd rather wait to get
this extension in i965 than add more work to that particular task. The
first thing I'd have to do is... convert the ATIfs->Mesa IR converter to
ATIfs->NIR. :)
> -ilia
More information about the mesa-dev
mailing list