Time to revert bad commits?
I'm afraid we are out of time to fix bugs that introduces major d3d regressions. I've yet to see a single patch that addressed those issues. It's time to start reverting bad commits and testing the results. The bugs I'm talking about: Bug 10580 Bug 11584 Bug 12794 Bug 13101 Vitaliy.
I'm afraid we are out of time to fix bugs that introduces major d3d regressions. I've yet to see a single patch that addressed those issues.
It's time to start reverting bad commits and testing the results.
The bugs I'm talking about: Bug 10580 Bug 11584 Bug 12794 Bug 13101
Vitaliy.
Regarding 13101 that bug is an Nvidia driver bug. The code itself is really correct and there are more cases in which the issue gets triggered (not just multisampling). I need more testers (I added comments to the bug) to reproduce it on different systems, games and driver versions so that I can create a good bug report to submit to Nvidia. Roderick -- GMX startet ShortView.de. Hier findest Du Leute mit Deinen Interessen! Jetzt dabei sein: http://www.shortview.de/?mc=sv_ext_mf(a)gmx
Am Montag, 2. Juni 2008 08:11:13 schrieb Roderick Colenbrander:
The bugs I'm talking about: Bug 10580 Sorry that I did not have time yet to look into that, but instead of ranting you could also look at the GLSL docs and make a patch. Renaming the local constants from LC to LVC for vertex and LPC for pixel shaders, and then loading the uniforms after program linking should fix the issue for GLSL. In ARB, just read them from program local environment and load the constants after compiling the shader.
Stefan Dösinger wrote:
Am Montag, 2. Juni 2008 08:11:13 schrieb Roderick Colenbrander:
The bugs I'm talking about: Bug 10580
Sorry that I did not have time yet to look into that, but instead of ranting you could also look at the GLSL docs and make a patch. I did see a precision patch by Vitaliy fixing this problem, which was not applied, presumably since there is a better way to fix this. Renaming the local constants from LC to LVC for vertex and LPC for pixel shaders, and then loading the uniforms after program linking should fix the issue for GLSL. In ARB, just read them from program local environment and load the constants after compiling the shader.
The patch being reverted seems interesting - it basically undoes a prior effort to get local constants into the same namespace as global ones due to relative addressing. I assume this is done for performance reasons, is this correct ? I see your patch is special-case disabled in the case of relative addressing. Do we really want to support multiple constant loading code paths for performance savings ? Ivan
Am Montag, 2. Juni 2008 11:57:21 schrieb Ivan Gyurdiev:
The patch being reverted seems interesting - it basically undoes a prior effort to get local constants into the same namespace as global ones due to relative addressing. I assume this is done for performance reasons, is this correct ? I see your patch is special-case disabled in the case of relative addressing. Do we really want to support multiple constant loading code paths for performance savings ? Yes, it is for performance reasons, and yes, we want the multiple paths. It may be a small, non-algorithmic improvement, but those small improvements are where you get the performance from in 3D rendering.
The reason for this is not only avoiding reloading the local constants each shader change or constant change, but by moving the local constants away from the global constant space we don't have to reload unchanged global constants due to a local constant change. On MacOS I achived a 5-15% performance gain with those constant loading optimizations, depending on the app, this is quite a lot. On the Linux Nvidia driver it was smaller, around 2-3% because the constant loading seems to be more efficient there.
On Monday 02 June 2008 12:04:54 Stefan Dösinger wrote:
Am Montag, 2. Juni 2008 11:57:21 schrieb Ivan Gyurdiev:
The patch being reverted seems interesting - it basically undoes a prior effort to get local constants into the same namespace as global ones due to relative addressing. I assume this is done for performance reasons, is this correct ? I see your patch is special-case disabled in the case of relative addressing. Do we really want to support multiple constant loading code paths for performance savings ?
Yes, it is for performance reasons, and yes, we want the multiple paths. It may be a small, non-algorithmic improvement, but those small improvements are where you get the performance from in 3D rendering.
The reason for this is not only avoiding reloading the local constants each shader change or constant change, but by moving the local constants away from the global constant space we don't have to reload unchanged global constants due to a local constant change.
On MacOS I achived a 5-15% performance gain with those constant loading optimizations, depending on the app, this is quite a lot. On the Linux Nvidia driver it was smaller, around 2-3% because the constant loading seems to be more efficient there. The shader compiler can also do better optimization with hard-coded constants (particularly if the constants are 0 or 1). I've seen the nvidia-compiler remove entire code paths because of this in a hl2-shader.
Am Dienstag, 3. Juni 2008 10:02:26 schrieb Fabian Bieler:
The shader compiler can also do better optimization with hard-coded constants (particularly if the constants are 0 or 1). I've seen the nvidia-compiler remove entire code paths because of this in a hl2-shader. Really? Do you have any evidence for this? In that case we might want to use Vitaliys original patch which just changed the precision of the printed float, since the patches I just sent break this ability. Henri and I considered them as theoretical, since the HLSL compiler or shader author would have removed those codepaths already
On Tuesday 03 June 2008 10:42:39 you wrote:
Am Dienstag, 3. Juni 2008 10:02:26 schrieb Fabian Bieler:
The shader compiler can also do better optimization with hard-coded constants (particularly if the constants are 0 or 1). I've seen the nvidia-compiler remove entire code paths because of this in a hl2-shader.
Really? Do you have any evidence for this? In that case we might want to use Vitaliys original patch which just changed the precision of the printed float, since the patches I just sent break this ability. Henri and I considered them as theoretical, since the HLSL compiler or shader author would have removed those codepaths already I did some testing using the Half-Life 2 intro and outro (dxlevel 81). Attached is a spreadsheet with the instruction and register counts of the compiled shaders (aquired via __GL_WriteProgramObjectAssembly). The difference is not as big as I expected (I stumbled over one of the more significant cases (fasm_164_0.txt of the outro: 13 vs. 9 instructions) some time ago and made a wrong extrapolation ;-)).
Am Dienstag, 3. Juni 2008 13:01:11 schrieb Fabian Bieler:
I did some testing using the Half-Life 2 intro and outro (dxlevel 81). Attached is a spreadsheet with the instruction and register counts of the compiled shaders (aquired via __GL_WriteProgramObjectAssembly). The difference is not as big as I expected (I stumbled over one of the more significant cases (fasm_164_0.txt of the outro: 13 vs. 9 instructions) some time ago and made a wrong extrapolation ;-)). I guess it is worth investigating this further, but after Wine 1.0. It would be fairly easy to load the stringable floats into the program string and keep the ones where this would cause precision loss loaded via glUniform4f. All we have to do is to set a flag per local constant and test this flag in two places
Hello Vitaliy, 2008/6/2 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
I'm afraid we are out of time to fix bugs that introduces major d3d regressions. I've yet to see a single patch that addressed those issues.
It's time to start reverting bad commits and testing the results.
The bugs I'm talking about: Bug 10580 Bug 11584 Bug 12794 Bug 13101 In general, you can't revert patches as that may have other side effects. Usually while they can cause regressions they also might make issues reoccur which the patch is trying to fix, blindly reverting is not the answer.
Cheers, Maarten.
participants (6)
-
Fabian Bieler -
Ivan Gyurdiev -
Maarten Lankhorst -
Roderick Colenbrander -
Stefan Dösinger -
Vitaliy Margolen