On Mon, Nov 14, 2011 at 2:49 PM, Vadim Girlin <vadimgir...@gmail.com> wrote:
> On Mon, 2011-11-14 at 05:22 -0800, Jose Fonseca wrote:
>>
>> ----- Original Message -----
>> > 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,
>>
>> The way to fix this is to allocate a consecutive range of temps at start,
>> when there are indirect writes to output registers and at least one read.
>>
>> > 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?
>>
>> The drawback of doing this is that TGSI will look even more different
>> between drivers. This means that when somebody makes a change to the state
>> tracker, and tested with one driver, it may break other drivers. This also
>> means that comparing a driver to llvmpipe/softpipe will be less meaningfull,
>> as different paths will be taken in the state tracker.
>>
>
> I think if it's needed to compare the drivers, than it's possible to
> switch of the the cap for debugging. I see the drawbacks, but this is
> also about performance. Currently with r600g (and possibly with other
> drivers) we have to spend some time for unneccessary shader
> modification, to get less efficient shader code as a result.
I am 100% sure nobody will turn on/off CAPs just to test something.
I gotta agree with José on this one.
The shader backend in a driver is primarily responsible for the
quality of final shader code, not the state tracker, although the
state tracker can help a lot.
r600g already allocates outputs in the temporary area, so it's not
like remove_output_reads would make any difference for that driver,
right?* Why not fix remove_output_reads such that it works with
indirect addressing? That would also fix _all the other drivers_ which
don't support output reads.
* Lack of proper register allocation is not an excuse, especially if
new temporaries are allocated by a driver for whatever reasons.
Marek
>
> By the way, which drivers do not support reading outputs? I haven't done
> a full piglit run with llvmpipe, but IIRR the single test mentioned
> above was also fixed for llvmpipe without this output replacement.
>
> Vadim
>
>> I'm not sure if for this particular issue a new cap is worth or not. Just
>> pointing out that there are downsides of breaking orthogonality between
>> state tracker and driver. It should not be done lightly.
>>
>>
>> Jose
>
>
>
> _______________________________________________
> 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
