[Mesa-dev] [PATCH] gallium: add double opcodes and TGSI execution (v2.1)

[Dave Airlie]

>> +   // fprintf(stderr, "%f %f\n", src[0].d[0], src[1].d[0]);
>
>
> leftover debug print

dropped,

>> enum tgsi_exec_datatype {
>>     TGSI_EXEC_DATA_FLOAT,
>>     TGSI_EXEC_DATA_INT,
>> -   TGSI_EXEC_DATA_UINT
>> +   TGSI_EXEC_DATA_UINT,
>> +   TGSI_EXEC_DATA_DOUBLE,
>
>
> One comma too many

fixed,


>> +   if (dtype)
>> +      return;
>> +
>
>
> Should probably note that modifiers are handled for doubles by caller.

Added a comment,


>> +
>> +   /* XXX: That won't work for operand modifiers.
>> +    */
>
>
> Will work due to return in fetch_source_d for last function arg=true?

dropped old comment.

>
>> +
>> +   /* XXX: That won't work for saturate modifiers.
>> +    */
>
>
> Saturate handled above, and early break out before saturate application in
> store_dest_optsat(), so comment outdated?

dropped comment,

>> +   union tgsi_double_channel src;
>> +   union tgsi_double_channel dst;
>> +
>> +   if ((inst->Dst[0].Register.WriteMask & TGSI_WRITEMASK_XY) ==
>> TGSI_WRITEMASK_XY) {
>> +      fetch_double_channel(mach, &src, &inst->Src[0], TGSI_CHAN_X,
>> TGSI_CHAN_Y);
>> +      op(&dst, &src);
>> +      store_double_channel(mach, &dst, &inst->Dst[0], inst, TGSI_CHAN_X,
>> TGSI_CHAN_Y);
>> +   }
>> +   if ((inst->Dst[0].Register.WriteMask & TGSI_WRITEMASK_ZW) ==
>> TGSI_WRITEMASK_ZW) {
>> +      fetch_double_channel(mach, &src, &inst->Src[0], TGSI_CHAN_Z,
>> TGSI_CHAN_W);
>> +      op(&dst, &src);
>> +      store_double_channel(mach, &dst, &inst->Dst[0], inst, TGSI_CHAN_Z,
>> TGSI_CHAN_W);
>> +   }
>
>
> assert on any other write mask?

I'm not sure what best way to handle that is, probably a good plan to
assert alright, I'll think about it.

>> +      if (dst_datatype == TGSI_EXEC_DATA_UINT) {
>> +         first_dest_chan = (wmask & TGSI_WRITEMASK_X) ? TGSI_CHAN_X :
>> TGSI_CHAN_Y;
>> +         second_dest_chan = -1;
>
>
> passing -1 to store_double_channel() taking uint, think some paths try to
> use that as array index without checks and will blow up.

I can't see any paths doing that, since I wrote the code to operate like this.
>> +#define TGSI_OPCODE_DRCP                208 /* eg, cayman */
>> +#define TGSI_OPCODE_DSQRT               209 /* eg, cayman also has DRSQ
>> */
>
>
> Adding DRSQ seems like a good idea, at least for graphics with floats its
> more frequent than SQRT.

Yes I agree with that now, I'll add it in next version.

Dave.