Implemented Math functions, automated headless graphics device testing, numerous bug fixes and improvements

[thargy]

Fixes #69, #70, #71, and adds support for System.MathF #65 (except some overloads of Round and IEEERemainder) however, testing not currently possible as MathF is not included in the current build framework. Added same methods to ShaderBuiltins making them readily available in all projects, and these are tested (except on Metal which I don't have access to).

Added support for true end->end testing, including compiling and running compute shaders using Veldrid. This can be seen in action in ShaderBuiltinsTests.cs, which is currently disabled due to inconsistencies on some backends, to re-enable set the SkipReason variable to null at the top of the file, for full breakdown of CPU/GPU failures by method.

Added new FactAttributes to auto-ignore tests that are not supported on the current build environment.

Enhanced TypeSizeCache now auto-detects non-blittable structs, is faster, and removes several infinite-recursion traps.

Began support for constructors & instance methods (#72) by allowing constructor bodies to be passed around as well as method bodies.

TODO: mod/fmod need researching more (see comments). Trig functions, particularly hyperbolic ones and inverse one, are woefully inconsistent between GPU/CPU due, in large part, to rounding issues.

Although, I've currently turned off the end->end tests, that is to allow the code to be tested on Metal backends, etc., before forcing onto the CI server. I believe this is now of sufficient quality to merge in as it is largely correct and ready for review, and adds a lot of enhancements. It was necessary to bundle quite a lot together to get end->end working and further fixes/enhancements shouldbe more granular.

[mellinoe]

By the way, the test output tends to be a bit noisy with messages about skipped tests. Is there a way that we can force the "skip" messages to get combined? On macOS, 80+ tests are skipped, so the output is really noisy, but all of the messages say the same thing. I'm not sure if we can do anything within Xunit's constraints.

[thargy]

• pow needs float parameters
• atan needs float parameters
  Since there's only two places that need float parameters, I'd say we just fix them up locally.

These are two methods that take a second parameter, and from the original code you linked the cast is only applied to this second parameter. The metal specification says these method can take multiple input types, so I guess the issue was an ambiguity was created if the second parameter was not obviously a float (Vectors would not be ambiguous) and therefore was not gauranteed to match the first parameters type.

The existing code checks if the first param is a float and then ensures the second parameter is unambiguously a float. It also checks if the typeName is System.Mathf, but this seems redundant as (to my knowledge) there is only one MathF.Pow overload and it already requires float. I can't use the same logic as we also support Pow and Atan in the builtins now.

To add complexity, in the built in methods, we have one Atan method with overloads that accept 1 or 2 parameters (OpenGL style), whereas MathF has Atan and Atan2 to discriminate between them (HLSL/Metal style). Also, according to the spec, unlike OpenGL, Metal follows the HLSL and MathF approach of having two seperate methods rather than an overload, so there was another bug which I've hopefully rectified as it was mapping the two parameter version to Atan (like OpenGL).

You can see the changes in 6be9b0e, however, I can't test as I don't have access to Metal.

[thargy]

By the way, the test output tends to be a bit noisy with messages about skipped tests. Is there a way that we can force the "skip" messages to get combined? On macOS, 80+ tests are skipped, so the output is really noisy, but all of the messages say the same thing. I'm not sure if we can do anything within Xunit's constraints.

The Attribute approach had the big advantage of skipping entire theories with a single message rather than skipping each test of the theory. That massively reduced skip messages. To be honest, I always use a GUI test runner so I never noticed any particular verbosity. I could go back and retry the attribute code to see if the CI server check (see 3324131) is sufficient to prevent the CI server from crashing now?

[mellinoe]

The latest version is getting close on macOS. There's still a few problems:

• The correction for Pow is based on the first parameter's type, but unfortunately it doesn't work for Pow(2, 10), at least not how it's currently implemented. ShaderGen doesn't keep track of the "true" parameter type (e.g. what the method actually accepts), but rather the type of the literal being passed in. So with Pow(2, 10), it thinks both types are int, and the float fixup isn't applied. We could either

  1. Check for int/uint/etc. as well
  2. Try to fix the parameter type tracking so it recognizes that the parameters should be floats

• ^ Same for atan

• Metal compilation is actually a two-step process. The shader code is compiled to bitcode using the metal tool, but then it needs to be assembled into a "metallib" file using the metallib tool. Currently, the compute shader end-to-end test only does the first step, which can't be loaded by Veldrid.

• macOS's version of OpenGL does not support compute shaders. This means we can't run the end-to-end test there. The Toolchain constructor should be augmented to check for GraphicsDevice.Features.ComputeShader, and only set "CreateHeadless" if that feature is present. Or, a different flag should be added that represents it. Up to you.

[thargy]

Try to fix the parameter type tracking so it recognizes that the parameters should be floats

Pretty sure that is ultimately the only real solution, as it will be necessary if you ever hope to consistently resolve OpenGLES' strict type checking. Further, there's an endless set of edge cases, any type that can be implicitly cast to a float will compile in the C#, but would fail a bulk type check. Looking to see what the method accepts is a far more robust solution.

Metal compilation is actually a two-step process. The shader code is compiled to bitcode using the metal tool, but then it needs to be assembled into a "metallib" file using the metallib tool. Currently, the compute shader end-to-end test only does the first step, which can't be loaded by Veldrid.

The ToolChain system can be extended readily, (for example, an early iteration seperated out Validation from Compilation). However, in this scenario it's probably considering these two steps as 'Compilation', rather than adding a new feature that wouldn't be used by any of the other tool chains. The ability to call an executable and grab the results is implemented in the private Execute method (see here). It is easy to evaluate the CompileResult result of the first step and pass it onto a second step by modifying the MetalCompile method here. However, not having access to a Mac or Metal this isn't something I could implement from my end.

macOS's version of OpenGL does not support compute shaders. This means we can't run the end-to-end test there. The Toolchain constructor should be augmented to check for GraphicsDevice.Features.ComputeShader, and only set "CreateHeadless" if that feature is present. Or, a different flag should be added that represents it. Up to you.

As far as I can see you can only extract the GraphicsDeviceFeatures from a GraphicsDevice instance, so one needs to be created before it can be checked. There are currently 14 flags, though I can see this extending in future. Therefore, it doesn't feel like it would logically fit in ToolChain. Instead, wherever certain graphics device features are required, they should probably be checked once the graphics device is created, and then an exception thrown if missing (with the exception type being set in the SkippableFactAttribute to cause the test to be marked as 'skipped' rather than 'failed'). I assume there are other reasons for using a headless graphics device than just compute shaders?

[thargy]

wherever certain graphics device features are required, they should probably be checked once the graphics device is created, and then an exception thrown if missing (with the exception type being set in the SkippableFactAttribute to cause the test to be marked as 'skipped' rather than 'failed'). I assume there are other reasons for using a headless graphics device than just compute shaders?

I've shown this is action in a930183.

[mellinoe]

As far as I can see you can only extract the GraphicsDeviceFeatures from a GraphicsDevice instance, so one needs to be created before it can be checked.

The Toolchain constructor already creates a GraphicsDevice instance to ensure the call will actually work (on this line), so I figured the check could just be done there. Checking inside of the method is also fine, but I thought this fit well into the "capability flags" concept. I don't have a strong opinion one way or the other)

I assume there are other reasons for using a headless graphics device than just compute shaders?

There's no reason to use headless GraphicsDevice instances except it avoids allocating the resources for an OS window. There's no difference from a regular device except a headless GraphicsDevice has no window, and it therefore has no MainSwapchain. You can use compute shaders in any GraphicsDevice.

[mellinoe]

I fixed the two-stage compilation problem in Metal, and pushed the commit here: fa445d4

Please feel free to cherry-pick that commit into your branch.

Pretty sure that is ultimately the only real solution, as it will be necessary if you ever hope to consistently resolve OpenGLES' strict type checking. Further, there's an endless set of edge cases, any type that can be implicitly cast to a float will compile in the C#, but would fail a bulk type check. Looking to see what the method accepts is a far more robust solution.

I agree. There's a few different cases that need to be handled, though, so it wasn't as simple as I was hoping. For method invocations, it seems straightforward -- we can get all of the parameter types from IMethodSymbol.Parameters, and we already have an IMethodSymbol. There are other cases that the invocation translation code handles, though (ctors, properties, etc.), and we need to handle those, too.

[thargy]

The Toolchain constructor already creates a GraphicsDevice instance to ensure the call will actually work (on this line),

I considered that but decided against for the following reasons:

• Doing the try...catch check always feels like a dirty hack. It's there at the moment as querying Veldrid directly is currently insufficient (it reports the Backend as supported even if it is incapable of creating a graphics device, and you have to create a graphics device before checking features), ultimately there should always be a mechanism other than 'see if it throws an exception'. At that point, I'd love to remove the hack.
• That check crashes the CI server (hence the need for the check for the CI environment variable) despite the try...catch, another reason to ultimately want to see the code replaced.
• I had no way of knowing if the GraphicsDevice.Features depended on the options specified when creating a graphics device, so I can't just cache the features after the initial creation.
• As above, what happens if (when we add support for Windowed graphics devices) I get different features from the two checks? (At the moment we haven't implemented this)
• If the features are consistent, why are they not exposed statically per backend, rather than per instance? That would allow you to query them without instantiating a graphics device?

Ultimately, I concluded that conflating the Graphics Device features with the ToolChain features is probably not a good idea. It is possible, should it prove necessary, to add skipping attributes in future that support checking for both types of features. I'm also a little surprised you chose a class of bools rather than a Flag enum, unless you plan for some features to be more descriptive in future? All that uncertainty made me nervous about just dumping it in there.

There's no difference from a regular device except a headless GraphicsDevice has no window, and it therefore has no MainSwapchain. You can use compute shaders in any GraphicsDevice.

Sorry, I was probably very unclear, my point was to question whether it was reasonable to say "because you don't support compute shaders you can't create headless graphics devices"? I didn't think that a was a valid stance to take? I was also under the impression you can use headless graphics devices for more than running compute shaders, like I thought you could still run a vertex/fragment shader and render to a texture, which could be useful for testing such shaders, even potentially in a CI environment?

[thargy]

Please feel free to cherry-pick that commit into your branch.

Done, though I've propogated the transpiled source into the second step so that it is returned in the output CompileResult and not lost during Lib creation. That way it remains consistent with the other tool chains.

I agree. There's a few different cases that need to be handled, though, so it wasn't as simple as I was hoping. For method invocations, it seems straightforward -- we can get all of the parameter types from IMethodSymbol.Parameters, and we already have an IMethodSymbol. There are other cases that the invocation translation code handles, though (ctors, properties, etc.), and we need to handle those, too.

As this would be a fairly major piece of work, is it worth concluding this pull-request and doing that as a seperate piece of work, only this pull-request already introduces a lot of changes to a lot of files? As not all of the new CPU-friendly methods are fully working yet it isn't really making the situation worse? To be honest, the current tree walks need reviewing as there's quite a bit of duplication and there are quite a few cases where concepts should be handled in a more encapsulated way (consider #62, #71, etc.)

Ideally, I'm keen to consider #78 again, so I can improve the CPU-GPU tests to being automatic and change them to compare across multiple GPUs as well as the CPU in one go, this will make achieving consistency more realistic. I understand pulling that from this branch, but most of the methods that are being moved aren't in the existing master branch anyway, and, although a breaking change, it is easy for downstream authors to refactor by including the new class where necessary.

[mellinoe]

As this would be a fairly major piece of work, is it worth concluding this pull-request and doing that as a seperate piece of work, only this pull-request already introduces a lot of changes to a lot of files?

That's fine, we just need to fix the current behavior on Metal, since it's not handling pow and atan correctly. If you'd like, then I can just make that change myself with a TODO marker.

[thargy]

That's fine, we just need to fix the current behavior on Metal, since it's not handling pow and atan correctly. If you'd like, then I can just make that change myself with a TODO marker.

If you could that would be helpful, as I'm not sure how best to fix it temporarily? The new ShaderBuiltins versions of Atan and Pow accept more overloads than System.MathF does.

I don't understand how the bug didn't exist for the original ShaderBuiltins.Pow, as that relied on purely checking the type of the second parameter - which as you suggest isn't good enough. System.MathF.Pow was OK, because it knew that method always required a float.

I'm happy to cherrypick a commit again? Obviously, I don't see the failure without metal.

[mellinoe]

I pushed another commit to my branch which gets me to green on macOS. If you cherry-pick that, I'll go ahead and run all my tests again and merge when green.

I ended up just always emitting the float fixup if it's a non-Vector overload (since that was easy to check). Hacky, but quick.

[thargy]

I ended up just always emitting the float fixup if it's a non-Vector overload. Hacky, but quick.

To be fair, that's probably the safest bet for now! I've cherry-picked it.

[mellinoe]

@thargy Everything looks good to me now. Would you have any problem if I squashed this when merging? There's quite a few "WIP" and intermediate state commits that I'd like to squash out, if you're not opposed.

[thargy]

@mellinoe knock yourself out! I should have done that already, I had a few commits when moving from work to home and vice versa.

[mellinoe]

@thargy Thanks for all the time and effort you dedicated to this. The improvements are well worth it!

[thargy]

@mellinoe thanks for the merge! Next step is to improve the consistency between the various backends and the cpu implementations.