[Mesa-dev] Allowing the reading of outputs for some drivers

[Alex Deucher]

On Tue, Nov 15, 2011 at 9:43 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>
>
> ----- Original Message -----
>> On Tue, Nov 15, 2011 at 8:58 AM, Jose Fonseca <jfonseca at vmware.com>
>> wrote:
>> >
>> >
>> > ----- Original Message -----
>> >> On 11/13/2011 03:06 AM, Vadim Girlin wrote:
>> >> > Hi,
>> >> >
>> >> > I found some problem with glsl_to_tgsi: remove_output_reads
>> >> > function.
>> >> > It's replacing outputs with temps, producing incorrect results
>> >> > with
>> >> > relative addressing. You can see it e.g. with
>> >> > "vs-varying-array-mat2-col-rd.shader_test". Here is a dump:
>> >> >
>> >> >> VERT
>> >> >> DCL IN[0]
>> >> >> DCL OUT[0], POSITION
>> >> >> DCL OUT[1], GENERIC[12]
>> >> >> DCL OUT[2], GENERIC[13]
>> >> >> DCL OUT[3], GENERIC[14]
>> >> >> DCL OUT[4], GENERIC[15]
>> >> >> DCL OUT[5], GENERIC[16]
>> >> >> DCL OUT[6], GENERIC[17]
>> >> >> DCL OUT[7], GENERIC[18]
>> >> >> DCL CONST[0..5]
>> >> >> DCL TEMP[0..1]
>> >> >> DCL ADDR[0]
>> >> >> IMM FLT32 {    1.0000,     2.0000,     3.0000,     4.0000}
>> >> >> IMM FLT32 {    5.0000,     6.0000,     7.0000,     8.0000}
>> >> >> IMM FLT32 {    9.0000,    10.0000,    11.0000,    12.0000}
>> >> >> IMM FLT32 {    0.0000,     1.0000,     0.0000,     0.0000}
>> >> >>    0: MUL TEMP[0], CONST[2], IN[0].xxxx
>> >> >>    1: MAD TEMP[1], CONST[3], IN[0].yyyy, TEMP[0]
>> >> >>    2: MAD TEMP[1], CONST[4], IN[0].zzzz, TEMP[1]
>> >> >>    3: MAD OUT[0], CONST[5], IN[0].wwww, TEMP[1]
>> >> >>    4: MOV OUT[2], IMM[0].xyyy
>> >> >>    5: MOV OUT[3], IMM[0].zwww
>> >> >>    6: MOV TEMP[0], IMM[1].xyyy
>> >> >
>> >> > OUT[2-7] is a "varying mat2x2[3] m;", OUT[4] is replaced with
>> >> > the
>> >> > temp
>> >> > in the instruction 6.
>> >> >
>> >> >>    7: MOV OUT[5], IMM[1].zwww
>> >> >>    8: MOV OUT[6], IMM[2].xyyy
>> >> >>    9: MOV OUT[7], IMM[2].zwww
>> >> >>   10: ARL ADDR[0].x, CONST[1].xxxx
>> >> >>   11: SNE TEMP[1], TEMP[ADDR[0].x].xyyy, CONST[0].xyyy
>> >> >
>> >> > Instruction 11 contains the read with the relative addressing
>> >> > using
>> >> > this temp, which is incorrect.
>> >> >
>> >> > I'm not sure how to fix it, but AFAICS at least for r600g this
>> >> > step
>> >> > could be skipped completely - r600 can read outputs without any
>> >> > problem, they are located in the general-purpose registers.
>> >> > Removing
>> >> > calls to remove_output_reads and assert(src.File !=
>> >> > TGSI_FILE_OUTPUT)
>> >> > in the ureg_emit_src produces correct result and test passes on
>> >> > evergreen (total number of fixed tests is about 60).
>> >> >
>> >> > Probably it makes sense to make this step optional and ask the
>> >> > driver
>> >> > whether to use it, if I'm not missing something?
>> >>
>> >> I'd say the right answer is to make a lowering pass that operates
>> >> on
>> >> the
>> >> GLSL IR, *NOT* on TGSI.
>> >
>> >> The st_glsl_to_tgsi does a lot of operations
>> >> on
>> >> TGSI IR that would be much easier and be much better suited to the
>> >> higher-level IR.
>> >
>> > Yes. I seldom touched the st_foo_to_tgsi.c but always thought to my
>> > self that IR manipulation should had happened before the TGSI
>> > translation.  However it was not clear if things were done as they
>> > are because of unfamiliarity with glsl internals, or some
>> > limitation in glsl passes.
>> >
>> > For example, reusing temporaries registers (so that the total
>> > number of temporaries is minimized), could this be done in a glsl
>> > pass?
>>
>> The tricky part is that we use TGSI for more than just GLSL.  The
>> pipe
>> video stuff and eventually compute, etc.
>
> Sure. But I don't think the TGSI manipulating code in st_mesa_to_tgsi.c / st_glsl_to_tgsi.cpp is used for those paths. Or was that the intention?

Not that I know of.  I was just making a general statement.

Alex