-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto generate tests #80
Conversation
…o build into compute shaders for rapid testing and comparison between CPU & GPU backends.
Created outline structure for new way to create test data and test CPU implementations.
…lts to show rate of inconsistency.
Corrrected format in TestTimer overloads to only show 2 decimal places. Improved formatting of TestBuiltins test output.
Failure analysis now gives much more insightful information by grouping results together, making it much easier to see where a failure is occuring.
…ide more control over tests. Removed old ShaderBuiltinTests code and cleanedup ready for pull request.
Hi @mellinoe, is there any more info you need for this? |
@thargy Sorry, this has been on my list. I'll try to get to it later today or tomorrow. |
No problems, just wasn't sure if you'd clocked it. I've merged the latest master changes you pushed. |
testSets.Select(t => t.Backend).Where(b => b != null).ToArray(), | ||
null, | ||
null, | ||
csFunctionName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "function name" used here should actually just be the function name alone (not including type), because this is passed through to Veldrid. So it should just be "CS". This causes the Metal tests to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to just "CS" threw:
ShaderGen.ShaderGenerationException : The name passed to computeFunctionName must be a fully-qualified type and method.
at ShaderGen.ShaderGenerator..ctor(Compilation compilation, LanguageBackend[] languages, String vertexFunctionName, String fragmentFunctionName, String computeFunctionName, IShaderSetProcessor[] processors) in D:\Source Control\ShaderGen\src\ShaderGen\ShaderGenerator.cs:line 126
at ShaderGen.Tests.AutoGenerated.BuiltinsTests.TestBuiltins() in D:\Source Control\ShaderGen\src\ShaderGen.Tests\AutoGenerated\BuiltinsTests.cs:line 149
So this may be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, maybe I commented on the wrong place. ShaderGen indeed wants the fully-qualified name, because it needs all that info to locate the function. When you pass the actual shader code to Veldrid, on the other hand, you should just pass the actual function name ("CS"). I thought the line I commented on was the variable that ultimately goes into Veldrid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you, I just spotted that and have fixed in latest push.
string csFunctionName, | ||
ITestOutputHelper output) | ||
{ | ||
if (Interlocked.CompareExchange(ref _executed, 1, 0) > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this is necessary. It doesn't look like the tests are executed in parallel, or even more than once anyways. Is there something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, soz, force of habit, most of the code I write is multi-threaded, so I'm just used to implementing barriers using this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been changed anyway to fix an issue on compilation failure.
@@ -0,0 +1,2 @@ | |||
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -672,7 +672,7 @@ private static CompileResult FxcCompile(string code, Stage stage, string entryPo | |||
private static readonly Lazy<string> _glslvPath = new Lazy<string>( | |||
() => | |||
{ | |||
// First, try to launch from the current environment. | |||
// Execute, try to launch from the current environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an intentional change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops
src/ShaderGen.Tests/TestUtil.cs
Outdated
for (int i = 0; i < floatCount; i++) | ||
{ | ||
floats[i] = (float)((random.NextDouble() * 2.0 - 1.0) * Math.Pow(2.0, random.Next(minMantissa, maxMantissa))); | ||
//floats[i] = (float)(random.NextDouble() * floatRange * 2f) - floatRange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/// <param name="decimalPlaces">The number of decimal places between 0 and 16 (ignored for bytes).</param> | ||
/// <param name="breakPoint">The break point between 0 and 1024 (or 0D to base on decimal points).</param> | ||
/// <returns>System.String.</returns> | ||
public static string ToMemorySize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need all of these overloads? If not, let's just include the one(s) we actually use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it was a cut'n'paste from my other project, I figured I'd put them in here in case they were useful in future. I'll remove the unused overloads.
src/ShaderGen.Tests/Resource.resx
Outdated
@@ -0,0 +1,178 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this resource file is pretty overkill for what we need, and it's quite bloated (plus the backing file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's auto-generated in VS, and is the way I've always handled big strings. I'll submit an alternate design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There's a bit more complexity here than I'd like, but hopefully that will pay off in the future as we work with the codebase more. If you fix the compute shader name (left a comment about it), then all the tests pass for me on my systems. There's a few other misc. things that should be fixed, but overall it looks to be in an okay state. |
… makes it easier to implement a check that runs for each element in a vector. Added ShaderBuiltins.FMod. Mod made consistent across backends (as distinct from FMod), by implementing correctly in HLSL (and CPU). Made Pow and Cbrt implementations consistent by using Abs of 1st parameter. This is how Vulkan does it, whereas the other backends all return NaN. As we can't consistently return NaN on backends, I've adopted Vulkan's approach. Corrected CPU Clamp, Lerp and SmoothStep implementations by removing checks (makes consistent with GPU implementations). Corrected CPU Clamp implementation.
…ped from analysis (before it would cause it to show as different), allowing the tests to continue if at least two backends can be compared.
…ads. Minor tweaks as requested in mellinoeGH-80
Though it might appear a little complicated I've tried to make it encapsulated and it can be easily used to test other methods, and, eventually, extended to test GPU-only methods (comparing between backends only and not CPU). I think it'll pay off as it has taken me about an hour to correctly identify and fix inconsistencies between the various backends, you can see the changes in f9a7b57. The Builtins are now consistent across all backends (and CPU). As it is now testing 1000 random iterations for each of the 165 methods that are implemented on GPU & CPU across all backends, writing manual tests would have been significantly more time consuming and prone to missing things. Only Atanh is currently showing problems on Direct3D. |
…S method as CS is now not called from code directly.
@mellinoe I believe I've implemented all your suggestions, and with the fix of I haven't, however, been able to test on metal, so you'll need to look at the |
There's a couple of problems on Metal with the latest changes.
The cast (UInt3->int) fails on Metal otherwise.
|
I'm also hitting a crash when the tests complete on Windows:
|
Fixed packed vector accesor logic in AddCheck on Metal.
Here's what my fix for AddCheck looks like:
|
I already pushed: return
$"{shaderType}({string.Join(",", Enumerable.Range(0, elementCount).Select(a => check.Replace("`", $"[{a}]")))})"; Which should be equivalent (and dropped _vectorAccessors).
I've never seen that myself, however reviewing the code suggests a possibility. On exit, if one of the tools is still running the test process will start shutting down and disposing objects. The I've pushed a change that will make sure the process is disposed prior to the waithandles being disposed. Hopefully that will, at the very least, de-register the event handlers and prevent the error you're seeing be posisble. |
I ran it a few times and, while it seems to occur less frequently, it still crashes occasionally (2/6 times currently). Metal seems to be good now -- no problems there any more. |
What test runner are you using on Windows, I'm just running in VS using the R# runner?
Are there any failures reported in
|
I'm just running |
@thargy Ah, I didn't realize there could be failures while the test still passed. Here's what it outputs: Analysing results for failures took 824.3ms ● Vector4 ShaderGen.ShaderBuiltins.Mod(Vector4 a, Vector4 b) failed 950/1000 (95%). Mod(<-0.1473419, -0.06606695, 3.790649, -3.812556>, <-0.5484863, 1.955159, 0.2919904, -0.2313264>) Mod(<-3.672568, 3.733981, -3.982012, 0.04768488>, <0.08522116, -0.6115583, 2.423151, -0.1089887>) Mod(<-3.337058, 3.664641, -0.04931281, -0.2034206>, <0.1302528, 0.3109234, -1.64931, -0.7285025>) Mod(<0.06146766, 0.2269077, -0.08715918, 0.7744274>, <-0.1101076, 0.1933157, -0.08376742, -1.092888>) Mod(<0.2180625, -0.6099011, -0.1890869, -0.1876781>, <0.09211984, -0.14133, 0.2206808, 0.320484>) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ● Vector4 ShaderGen.ShaderBuiltins.Mod(Vector4 a, Single b) failed 936/1000 (93.6%). Mod(<-1.332414, -0.1519372, 0.3580664, 0.2030616>, -0.2266406) Mod(<-0.2432121, -0.4899388, -0.1968114, 1.390035>, 0.7634823) Mod(<1.875447, 0.08230057, 0.4035302, -0.04449578>, -1.059721) Mod(<-0.9921335, 0.2405954, 0.02952472, -0.1325384>, -0.3359732) Mod(<0.7359577, -1.616542, 0.4873287, -0.447501>, 0.6781166) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ● Vector3 ShaderGen.ShaderBuiltins.Mod(Vector3 a, Vector3 b) failed 887/1000 (88.7%). Mod(<-0.06014774, -0.9836, 0.02765148>, <0.1230681, 0.8742101, -0.1192536>) Mod(<0.0284753, -0.1399699, 0.07027472>, <-0.6422678, -0.2835048, 0.1584362>) Mod(<3.830129, -0.2082705, -0.3100274>, <-0.7649199, 3.870798, -0.344797>) Mod(<0.1018435, -0.1338812, 0.1493706>, <0.1147475, 3.891926, -1.851492>) Mod(<-0.1772397, 0.1387893, -0.1407941>, <-0.8813902, 0.6458945, 0.04376643>) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ◕ Vector3 ShaderGen.ShaderBuiltins.Mod(Vector3 a, Single b) failed 868/1000 (86.8%). Mod(<-1.485478, 1.124717, -0.3238766>, -0.259662) Mod(<-0.05152696, 0.1360926, 0.1211376>, 0.1381756) Mod(<0.02352959, -0.5231687, 1.2423>, -0.1055625) Mod(<1.713617, 1.400487, -0.1511525>, 0.3490644) Mod(<1.736485, -0.2102596, -1.810122>, -1.510112) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ◕ Vector2 ShaderGen.ShaderBuiltins.Mod(Vector2 a, Vector2 b) failed 757/1000 (75.7%). Mod(<-0.06524262, 0.1453021>, <0.1248452, -0.1563947>) Mod(<-0.24629, -0.1658617>, <-0.1526749, 0.08190227>) Mod(<0.1217519, -0.08054987>, <-0.06699717, 0.1960435>) Mod(<-0.02917885, 0.3808856>, <0.423089, -0.2923595>) Mod(<-1.068448, 0.1429756>, <0.2403389, -0.2420461>) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ◕ Vector2 ShaderGen.ShaderBuiltins.Mod(Vector2 a, Single b) failed 747/1000 (74.7%). Mod(<-0.4987893, 2.1514>, 0.3335225) Mod(<-0.04075132, 0.1077888>, -0.8046364) Mod(<3.513057, -0.3537728>, 0.4259948) Mod(<0.09067725, -1.519614>, 0.9782577) Mod(<0.3394809, -0.05812054>, 0.1904949) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ◑ Single ShaderGen.ShaderBuiltins.Mod(Single a, Single b) failed 509/1000 (50.9%). Mod(0.523179, -0.3522496) Mod(-0.8329723, 0.1716798) Mod(-0.71939, 0.09926502) Mod(-0.9161512, 0.2987621) Mod(-0.2505285, 0.4783684) … ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
If run this repeatedly now without any failures locally :(. As for metal, can you try changing line 55 in {nameof(ShaderBuiltins.Mod), SimpleNameTranslator("fmod")}, to {nameof(ShaderBuiltins.Mod), SimpleNameTranslator()}, in the first instance? |
Plain old
|
…dition. Add Metal mod operator.
Apparently, there is a '%' operator that might be equivalent so I've pushed some code for that, if not, it will be similar to the
Finally got a failure! I've pushed a commit with try...catch around the sets for the rare race condition. |
Metal's
|
That seems to work well enough.
|
…o build into compute shaders for rapid testing and comparison between CPU & GPU backends. Added ToMemorySize() to TestUtils for nicer test output. Created outline structure for new way to create test data and test CPU implementations. Now creates test data, and a result set based on executing on the CPU. Added TestTimer to make it easier to output timing message during long running tests. Runs test on all backends and does naive byte wise comparison of results to show rate of inconsistency. Added some more utilities methods to improve test output. Corrrected format in TestTimer overloads to only show 2 decimal places. Improved formatting of TestBuiltins test output. Move code into AutoGenerated folder, and split out classes to improve readability. Renamed Mappings.Methods to Mappings.MethodMaps and changed Methods to count field. Major refactor to make more OO and easier to understand. Failure analysis now gives much more insightful information by grouping results together, making it much easier to see where a failure is occuring. Added more flexible equality that is based on float comparison where possible. Added float generation control, and float comparison control, to provide more control over tests. Removed old ShaderBuiltinTests code and cleanedup ready for pull request. Added new 'AddCheck' methods to known functions implementations. This makes it easier to implement a check that runs for each element in a vector. Added ShaderBuiltins.FMod. Mod made consistent across backends (as distinct from FMod), by implementing correctly in HLSL (and CPU). Made Pow and Cbrt implementations consistent by using Abs of 1st parameter. This is how Vulkan does it, whereas the other backends all return NaN. As we can't consistently return NaN on backends, I've adopted Vulkan's approach. Corrected CPU Clamp, Lerp and SmoothStep implementations by removing checks (makes consistent with GPU implementations). Corrected CPU Clamp implementation. A failure to compile a backend will not cause the test set to be skipped from analysis (before it would cause it to show as different), allowing the tests to continue if at least two backends can be compared. Fixed issues with Open GL ES floats, and support for MathF.Pow overloads. Minor tweaks as requested in GH-80 Fixed issue with Open GL ES float cast bracket position. Correct to only pass method name into execution code, as per GH-80 comments. Removed resources file as per GH-80 request. Also removed DoCS method as CS is now not called from code directly. Fixed atanh implementaion in HLSL. Fixed bug with extracting thread ID in compute shader. Fixed packed vector accesor logic in AddCheck on Metal. Ensure waithandles are disposed after process by changing using order. Wrapped waithandle set's in try..catch to prevent occasional race condition. Add Metal mod operator.
@thargy I merged this manually and applied the suggested implementation for Metal's Thanks for the contribution! |
This pull request replaces the manual ShaderBuiltinsTest with a new auto-generated code test suite. The new test (
TestBuiltins
) accepts a list of methods that can run on both CPU & GPU and then builds the code for each available backend automatically. It then generates random test data and runs multiple iterations against the methods under test, storing the results. Finally, it analyses the differences between backends, and the CPU and produces a report similar to:Which clearly shows an issue in the way the CPU code for
SmoothStep
is implemented.Or:
From which it much easier to start diagnosing where issues are occurring, particularly as it shows the values that resulted in failures occurring.