| Age | Commit message (Collapse) | Author |
|---|
|
* #include an absolute path didn't work - because paths were taken to always be relative.
* WIP: First pass in supporting output of line error information.
* Add support for lexing to better be able to indicate SourceLocation information.
* Fix lexer usage in DiagnosticSink in C++ extractor.
* Update diagnostics tests to have line location info.
* Fixed test expected output that now have source location information in them.
* Better handling of tab.
* Fix test expected results for tabbing change.
* DiagnosticLexer -> DiagnosticSink::SourceLocationLexer
Added line continuation tests.
* Fix typo.
* Added String::appendRepeatedChar
* Change to rerun tests.
Co-authored-by: Tim Foley <tfoleyNV@users.noreply.github.com>
|
|
* #include an absolute path didn't work - because paths were taken to always be relative.
* Work around for issue with obfuscation (and lack of name hints) leading to names in output not being correctly uniquified.
* Improve appendChar
Remove unrequired memory juggling to scrub names.
* Remove test code.
* Small fixes in comments and method called.
* Remove linkage decoration on functions that are specialized.
* Obfuscation naming with specialization test.
* Fix instruction deletion.
Co-authored-by: Tim Foley <tfoleyNV@users.noreply.github.com>
|
|
* Fix existential specialization of mutable buffer loads.
* fix
Co-authored-by: Yong He <yhe@nvidia.com>
|
|
This change converts a large number of our existing tests to use the `ShaderObject` support that was added to the `gfx` layer.
In many cases, tests were just updated to pass `-shaderobj` and the result Just Worked.
In other cases, a `name` attribute had to be added to one or more `TEST_INPUT` lines.
For tests that did not work with shader objects "out of the box," I spent a little bit of time trying to get them work, but fell back to letting those tests run in the older mode.
Future changes to the infrastructure will be needed to get those additional tests working in the new path.
Along with the changes to test files, the following implementation changes were made to get additional tests working:
* Because the shader object mode uses explicit register bindings (from reflection), the hacky logic that was offseting `u` registers for D3D12 based on the number of render targets gets disabled (by another hack).
* The "flat" reflection information coming from Slang was not correctly reporting "binding ranges" for things that consumed only uniform data (which would be everything on CUDA/CPU), so it was refactored to properly include binding ranges for anything where the type of the field/variable implied a binding range should be created (even if the `LayoutResourceKind` was `::Uniform`).
* A few fixes were made to the CUDA implementation of `Renderer`, in order to get additional tests up and running. Most of these changes had to do with texture bindings, which hadn't really been tested previously.
In addition, a few changes were made that were attempts at getting more tests working, but didn't actually help. These could be dropped if requested:
* As a quality-of-life feature (not being used) the `object` style of `TEST_INPUT` line is upgraded to support inferring the type to use from the type of the input being set.
* Any `object` shader input lines get ignored in non-shader-object mode.
|
|
* Use "capability" system to select VKRT extension
Slang currently supports translation of ray tracing shader code to Vulkan GLSL code that uses the `GL_NV_ray_tracing` extension. A multi-vendor equivalent of that extension has been released as `GL_EXT_ray_tracing` and we want Slang to support that extension as well.
At the simplest, making the change from one extension to the other is just a matter of changing a few strings, since it does not appear that anything of significance was changed at the GLSL level (or even in SPIR-V). Where this gets trickier is when we have users who want us to support *both* extensions, and to be able to switch between them.
The solution we've implemented here more or less amounts to:
* If you don't tell the compiler which extension to use, it will default to `GL_EXT_ray_tracing` (the newer multi-vendor one).
* If you explicitly want the older extension, you can opt into it using the `-profile` option or via a new API for explicitly adding capabilities to your target.
Making that work required a few different kinds of changes:
* The options parsing and public API needed ways to add optional capabilities to a target.
* During GLSL code emit, we can check the capabilities that were added to the target to see if the `GL_NV_ray_tracing` extension was explicitly enabled and, if not, default to using the `GL_EXT_ray_tracing` names for things. This step is needed because some of the modifiers/attributes involved in the extension have to be handled explicitly in the code generator rather than implicitly as part of mapping intrinsic functions.
* We add two different translations to the relevant operatiosn in the stdlib, one marked with each of the extensions. If profile/capability-based overload resolution can be relied on to pick the right one, this should Just Work.
* Next, a bunch of work had to go into making capability-based overloading Just Work for the purposes of this change. There's been a nearly complete reworking of the implementation of `CapabilitySet` here to make it more suitable for our needs.
* The tests that were using ray tracing translation for Vulkan needed to be updated. For some of them I updated their baselines to use `GL_EXT_ray_tracing` so that they can test the new path. For others, I updated the command line for the test case so that it explicitly opts into using `GL_NV_ray_tracing`. The result is that we have some coverage of each extension. I would have liked to have each test run in both modes, but our pass-through glslang support doesn't support `-D` options, so I couldn't take that step easily.
This change does *not* add support for `GL_EXT_ray_query`, the extension that supports "DXR 1.1" style queries under Vulkan. Adding support for that extension should hopefully be a smaller step because it doesn't have the same multiple-extensions issue.
This change does *not* address a lot of possible avenues for improvement or cleanup around the capability system. It focuses only on those changes that are necessary to make the ray tracing feature work and leaves the rest for future work.
* fixup: infinite loop
* Comment-only change to retrigger TC build
|
|
* "Shader Toy" example and related fixes
This change introduces a new `shader-toy` example program that is primarily designed to show how Slang's features for type-based encapsulation and modularity can be applied to modularity for effects along the lines of those from `shadertoy.com`.
The Example
-----------
The example is being checked in with an example "toy" effect that I hastily put together, so that it would not be encumbered with any IP concerns. I wrote the effect using the shadertoy.com editor, so I can be sure it is valid GLSL. During bringup of the application I used a pre-existing and larger effect for testing, so some of the support code that was added is not being used at present.
The big-picture idea here is to have an exmaple that shows how to modularize things using Slang interfaces and generics, and then to use the Slang compiler API to manage the compilation, composition, specialization, and linking steps. For better or worse this leads to the sequence of API calls involved being much longer than what was in something like the `hello-world` example.
Future Work (Example)
---------------------
There is a lot of room for improvement and expansion here, so this should be viewed as a checkpoint of work in progress rather than something I'm claiming as a finalized demonstration of all we'd like to achieve. Areas for future work include:
* We need to copy the integration of "Dear, IMGUI" that was already done for the `model-viewer` example so that this example can have a UI.
* Now that the compilation flow is broken into all these additional steps, it should be possible to have the application load multiple effects as distinct modules, and then provide a UI for switching between them. The chosen effect module would be used to specialize the top-level shader(s) before kernel generation.
* The checked-in logic includes a compute shader that can execute an effect, but that hasn't been tested nor has it been wired up to any kind of UI. We should have a way to switch between multiple execution methods, with a goal of eventually including CPU execution.
* The "GLSL compatibility" code needs a lot of improvements before it is likely to be usable for a nontrivial number of shaders. Some of that work is waiting on Slang compiler fixes, though.
* We should consider allowing the individual "toy" effects to define their own uniform parameters and expose those via a UI and reflection. The catch in this case is not that this would be difficult to do, but that it would be a semantic change to how shader toy effects currently work.
The Compiler Fixes
------------------
Doing this work exposed a few bugs in Slang, and this change includes fixes for the ones that were quick to address.
We already had logic in `slang-check-shader.cpp` that was validating the entry points in a compile request - either by checking the explicitly-listed entry points, or by scanning for `[shader("...")]` attributes. The problem is that the routine that did that checking was not being invoked on all compiles. The logic that handled entry points was only being run for manual compiles using `SlangCompileRequest`, while anything using `import` or `loadModule` would ignore entry points. I refactored the relevant code into a subroutine that will be invoked in all compilation scenarios.
There were already `TODO` comments in `SpecializedComponentType` which made the point about how a specialized entry point like `myShader<YourType>` would need to properly show that it has dependencies on both the module that defines `myShader` *and* the module that defines `YourType`, while only the former was being handled at present. I went ahead and implemented the logic to scan the generic arguments for a specialized compoment type in order to determine what module(s) the arguments depend on (both type arguments and witness tables). With that change, using `IComponentType::link` on a specialized component will properly pull in the module(s) that the generic arguments come from.
In `slang-ir-legalize-types.cpp` we could run into assertion failures in debug builds because of code trying to legalize layout `IRAttr`s for fields or parameters with types that need legalization. In practice it is safe to skip these layout attributes, because legalization of the fields/parameters they pertain to would result in creation of entirely new layout attributes, and the old ones would then be unreferenced.
Future Work (Fixes)
-------------------
There are other compiler bugs that this work exposed, but which this change does not address. These will need to be resolved as part of subsequent changes:
* Slang allows for default-initialization of variables of a generic type. That is, given `<T : ISomething>` a user is allowed to declare `T x = {};` and the Slang front-end does not complain. Instead, this leads to an internal compiler error during IR lowering.
* The Slang `__init()` feature probably needs to be upgraded to a properly supported feature, and we probably need a way to make implementing default-initialization an easy thing (e.g., any `struct` type that has initial-value expressions for all its fields should automatically and implicitly satsify an `init();` requirement declared in an interface)
* Iniside an `__init()` definition, code has mutable access to members of the enclosing type, but for some reason the front-end is incorrectly treating `this` as immutable in those contexts. As a result you can write to `someField` but not `this.someField`.
* User-defined operator overloads flat out don't work (which isn't surprising given that no clients have decided to use them yet, and we have no test coverage for them). This is actually due to the shadowing rules being used for lookup right now, so a fix for this issue is going to have far-reaching consequences around what overloads are visible where (and anything that impacts overload resolution is a big can of worms, including around performance).
* fixup: test case had missing main function
|
|
* Add shader object parameter binding to renderer_test.
* remove multiple-definitions.hlsl
* Fix cuda implementation.
Co-authored-by: Tim Foley <tfoleyNV@users.noreply.github.com>
|
|
Slang generates code that turns the implicit `this` parameter of a method into an explicit parameter. The logic that decides whether that parameter should be `inout` is a bit involved, and there was a bug where a generic method would lead to the use of an `in` modifier (the default) and override the `inout` modifier that was requested by the method itself.
This change fixes the logic to treat generic declarations in the parent chain of a leaf method as having no bearing on whether an implicit `this` parameter should be `inout` or not.
A test case is included that breaks with the old behavior, and demonstrates that a generic `[mutating]` method can now work correctly.
|
|
The basic problem here is that when a function has multiple declarations with matching signatures (e.g., a forward declaration and then a later definition with a body), the IR lowering logic would lower all declarations whenever the first one was encountered, but then would only register an IR value as the lowered version of the first declaration. Other matching declarations would then run the risk of being lowered again, and in the case where they included features like loops with break/continue labels, that would create the risk of keys getting inserted into certain dictionaries more than one, leading to exceptions.
This change ensures that when lowering a function that has multiple matching declarations to IR, we register an IR value for all of those declarations and not just the first.
I have added a test case that leads to a crash without this change, to ensure that we don't introduce a regression down the line.
|
|
* Enable all dynamic dispatch tests on CUDA.
* Fix expected cross-compile test results.
|
|
During semantic checking, the compiler used to link together `ExtensionDecl`s into a singly-linked list dangling off of the `AggTypeDecl` that they applied to. This approach made lookup relatively easy, because given a `DeclRef` to an `AggTypeDecl` one could easily find and walk the list of candidate extensions.
Unfortunately, the simple approach has two major strikes against it:
* First, as we recently ran into, it creates a lifetime/ownership problem, in cases where the `ExtensionDecl` is outlived by the `AggTypeDecl` it applies to. This creates the one and only place in the compiler today where an "old" AST node might point to a "new" AST node, and it resulted in use-after-free problems in client code.
* Second, the scoping of `extension`s ends up being completely wrong. All of the `extension` methods on a type end up being visible in all cases, instead of just in the context of modules where the `extension` itself is visible. The comparable feature in C# (static extension methods) is careful to not make scoping mistakes like this. The Swift langauge has loose scoping for `extension` more akin to what we have in Slang today, but the maintainers seem to consider it a misfeature.
This change attempts to clean up both issues by changing the way that extension declarations are stored. There are two main pieces:
1. The primary "source of truth" for extension lookup has been moved to the `ModuleDecl`, where a module is responsible for storing a cache of the extensions declared within that module (keyed by the declaration of the type being extended). This cache is updated at the same point where the old code would mutate the AST node being depended on.
2. A secondary aggregated cache is added to the `SharedSemanticsContext` used during semantic checking. This cache includes entries from across multiple modules, and is intended to be invalidated and rebuilt on demand if new modules are added during checking.
Access to the candidate extensions has now been put behind subroutines that require a semantics-checking context to be passed in (there was always one available in contexts that care about extensions).
In addition, the operation for looking up members including those from extensions was refactored heavily to involve internal rather than external iteration and, more importantly, was changed so that it actually tests whether the `ExtensionDecl`s it loops over apply to the type in question, rather than blindly letting extensions members be looked up in ways that don't make sense.
There are three test cases added here to confirm aspects of the fix:
* First, I added a test that reproduces the crash that was being seen, so that we have a regression test for the fix.
* Second, I added a basic semantic-checking test to confirm that an `extension` from an `import`ed module is still visible/usable, to confirm that I didn't break existing valid uses of extensions.
* Third, I added a diagnostic test that ensures that we correctly ignore extensions that should not be visible in a given context as a result of `import` declarations.
Co-authored-by: jsmall-nvidia <jsmall@nvidia.com>
|
|
contains an array (#1448)
* This code is disabled, it was part of the optimization `Specialize function calls involving array arguments. (#1389)` on github.
It is disabled here because it causes a problem when a struct is passed to a function that contains a structured buffer *and* an array. It is specialized on the struct type, and so those types become parameters to the function. If the struct contains a structured buffer this is a problem on GLSL/VK based targets because currently structured buffers cannot be function parameters.
The fix for now is to just disable this optimization.
* Fix typo in name of test expected values.
|
|
If somebody defines two `struct` types with the same name:
```hlsl
struct A {}
// ...
struct A {}
```
and then tries to use that name when specializing a generic function:
```hlsl
void doThing<T>() { ... }
// ...
doThing<A>();
```
then the Slang front-end currently crashes, which leads to it not diagnosing the original problem (the conflicting declarations of `A`).
This change fixes up the checking of generic arguments so that it properly fills in dummy "error" arguments in place of missing or incorrect arguments, and thus guarantees that the generic substitution it creates will at least be usable for the next steps of checking (rather than leaving null pointers in the substitution).
This change also fixes up the error message for the case where a generic application like `F<A>` is formed where `F` is not a generic. We already had a more refined diagnostic defined for that case, but for some reason the site in the code where we ought to use it was still issuing an internal compiler error around an unimplemented feature.
This chagne includes a diagnostic test case to cover both of the above fixes.
|
|
The Slang compiler was bit by a known issue when translating from SSA form back to straight-line code. Give code like the following:
int x = 0;
int y = 1;
while(...)
{
...
int t = x;
x = y;
y = t;
}
...
The SSA construction pass will eliminate the temporary `t` and yield code something like:
br(b, 0, 1);
block b(param x : Int, param y : Int):
...
br(b, y, x);
The loop-dependent variables have become parameters of the loop block, and the branchs to the top of the loop pass the appropriate values for the next iteration (e.g., the jump that starts the loop sends in `0` and `1`).
The problem comes up when translating the back-edge the continues the loop out of SSA form. Our generated code will re-introduce temporaries for `x` and `y`:
int x;
int y;
// jump into loop becomes:
x = 0;
y = 1;
for(;;)
{
...
// back-edge becomes
x = y;
y = x;
continue;
}
The problem there is that we've naively translated a branch like `br(b, <a>, <b>)` into `x = <a>; y = <b>;` but that doesn't work correctly in the case where `<b>` is `x`, because we will have already clobbered the value of `x` with `<a>`.
The simplest fix is to introduce a temporary (just like the input code had), and generate:
// back-edge becomes
int t = x;
x = y;
y = t;
This change modifies the `emitPhiVarAssignments()` function so that it detects bad cases like the above and emits temporaries to work around the problem. A new test case is included that produced incorrect output before the change, and now produces the expected results.
A secondary change is folded in here that tries to guard against a more subtle version of the problem:
for(...)
{
...
int x1 = x + 1;
int y1 = y + 1;
x = y1;
y = x1;
}
In this more complicated case, each of `x` and `y` is being assigned to a value derived from the other, but neither is being set using a block parameter directly, so the changes to `emitPhiVarAssignments()` do not apply.
The problem in this case would be if the `shouldFoldInstIntoUseSites()` logic decided to fold the computation of `x1` or `y1` into the branch instruction, resulting in:
x = y + 1;
y = x + 1;
which would again violate the semantics of the original code, because now there is an assignment to `x` before the computation of `x + 1`.
Right now it seems impossible to force this case to arise in practice, due to implementation details in how we generate IR code for loops. In particular, the block that computes the `x+1` and `y+1` values is currently always distinct from the block that branches back to the top of the loop, and we do not allow "folding" of sub-expressions from different blocks. It is possible, however, that future changes to the compiler could change the form of the IR we generate and make it possible for this problem to arise.
The right fix for this issue would be to say that we should introduce a temporary for any branch argument that "involves" a block parameter (whether directly using it or using it as a sub-expression). Unfortunately, the ad hoc approach we use for folding sub-expressions today means that testing if an operand "involves" something would be both expensive and unwieldy.
A more expedient fix is to disallow *all* folding of sub-expressions into unconditional branch instructions (the ones that can pass arguments to the target block), which is what I ended up implementing in this change. Making that defensive change alters the GLSL we output for some of our cross-compilation tests, in a way that required me to update the baseline/gold GLSL.
A better long-term fix for this whole space of issues would be to have the "de-SSA" operation be something we do explicitly on the IR. Such an IR pass would still need to be careful about the first issue addressed in this change, but the second one should (in principle) be a non-issue given that our emit/folding logic already handles code with explicit mutable local variables correctly.
|
|
Static Method Calls
-------------------
The main fix here is for parsing of calls to static methods. Given a type like:
struct S
{
void doThing();
}
the parser currently gets tripped up on a statement like:
S::doThing();
The problem here is that the `Parser::ParseStatement` routine was using the first token of lookahead to decide what to do, and in the case where it saw a type name it assumed that must mean the statement would be a declaration.
It turns out that `Parser::ParseStatement` already had a more intelligent bit of disambiguation later on when handling the general case of an identifier (for which it couldn't determine the type-vs-value status at parse time), and simply commetning out the special-case handling of a type name and relying on the more general identifier case fixes the issue.
That catch-all case still has some issues of its own, and this change expands on the comments to make some of those issues clear so we can try to address them later.
Empty Declarators
-----------------
One reason why the static method call problem was hard to discover was that it was masked by the parser allowing for empty declarator. That is, given input like:
S::doThing();
This can be parsed as a variable declaration with a parenthesized empty declarator `()`.
Practically, there is no reason to support empty declarators anwhere except on parameters, and allowing them in other contexts could make parser errors harder to understand.
This change makes the choice of whether or not empty declarators are allowed something that can be decided at each point where we parse a declarator, and makes it so that only parsing of parameters opts in to allowing them.
By disabling support for empty declarators in contexts where they don't make sense, we make code like the above a parse error when it appears at global scope, rather than a weird semantic error.
A more complete future version of this change might *also* make support for parenthesized declarators an optional feature, or remove that support entirely. Slang doesn't actually support pointers (yet) so there is no real reason to allow parenthesized declarators right now.
One note for future generations is that using an emptye declarator on a parameter of array type can actually create an ambiguity. If the user writes:
void f(int[2][3]);
did they mean for it to be interpreted as:
void f(int a[2][3]);
or as:
void f(int[2][3] a);
or even as:
void f(int[2] a[3]);
The first case there yields a different type for `a` than the other two, but is also what we pretty much *have* to support for backwards compatibility with HLSL. Requiring all function declarations to include parameter names would eliminate this potentially confusing case.
Layout Modifiers
----------------
One of the above two syntax changes led to a regression in the output for a diagnostic test for `layout` modifiers (which are a deprecated but still functional feature from back when `slangc` supported GLSL input).
The original output of the test case seemed odd, and when I looked at the parsing logic I saw that an early-exit error case was leading to spurious error messages because it failed to consume all the tokens inside the `layout(...)`. Fixing the logic to not use an early-exit (and instead rely on the built-in recovery behavior of `Parser`) produced more desirable diagnosic output.
I changed the input file to put the `binding` and `set` specifiers on differnet lines so that the error output could show that the compiler properly tags both of the syntax errors.
|
|
These are steps towards a fix for the problem of not being able to call a static method as follows:
SomeType::someMethod();
One problem in the above is that the parser gets confused and parses an (anonynmous!) function declaration. This change doesn't address that problem, but *does* fix the problem that when checking fails to coerce `SomeType::someMethod` into a type it was triggering an unimplemented-feature exception rather than a real error message.
Another problem was that if the above is re-written to try to avoid the parser bug:
(SomeType::someMethod)();
we end up with a call where the base expression (the callee) is a `ParenExpr` and the code for handling calls wasn't expecting that. Instead, it sent the overload resolution logic into an unimplemented case that was bailing by throwing an arbitrary C++ exception instead of emitting a diagnostic.
This latter issue was fixed in two ways. First, the code path that failed to emit a diagnostic now emits a reasonable one for the unimplemented feature (this still ends up being a fatal compiler error). Second, we properly handle the case of trying to call a `ParenExpr` by unwrapping it and using the base expression instead, so that `(<func>)(<args>)` is always treated the same as `<func>(<args>)`.
|
|
* Added handling for empty switch body.
Added test for empty switch.
* Fix testing for case in switch.
|
|
The only big catch that I ran into with this batch was that I found the `float.getPi()` function was being emitted to the output GLSL even when that function wasn't being used. This seems to have been a latent problem in the earlier PR, but was only surfaced in the tests once a Slang->GLSL test started using another intrinsic that led to the `float : __BuiltinFloatingPointType` witness table being live in the IR.
The fix for the gotcha here was to add a late IR pass that basically empties out all witness tables in the IR, so that functions that are only referenced by witness tables can then be removed as dead code. This pass is something we should *not* apply if/when we start supporting real dynamic dispatch through witness tables, but that is a problem to be solved on another day.
The remaining tricky pieces of this change were:
* Needed to remember to mark functions as target intrinsics on HLSL and/or GLSL as appropriate (hopefully I caught all the cases) so they don't get emitted as source there.
* The `msad4` function in HLSL is very poorly documented, so filling in its definition was tricky. I made my best effort based on how it is described on MSDN, but it is likely that if anybody wants to rely on this function they will need us to vet our results with some tests.
|
|
This arose when a user tried to specialize the DXR 1.1 `RayQuery` type to a local variable:
```hlsl
RAY_FLAG rayFlags = RAY_FLAG_CULL_FRONT_FACING_TRIANGLES | RAY_FLAG_CULL_NON_OPAQUE;
RayQuery<rayFlags> query;
```
In this case, we issued an error around `rayFlags` not being a constant as expected, but then we also crashes later on in checking because the `DeclRef` that was being used for the type had a null pointer for the generic argument corresponding to `rayFlags`.
The main fix here was thus to add an `ErrorIntVal` case that can be used to represent something that should be an `IntVal` but where there was some kind of error in the input code so that the actual value isn't known to the compiler.
A secondary fix here is that we were issuing error messages about expecting a constant for a parameter like `rayFlags` there *twice*, and one of those times was during the `JustChecking` part of overload resolution (when we are not supposed to emit any diagnostics). I fixed that up by allowing the `DiagnosticSink` to be used to be passed down explicitly (and allowing it to be null), while also leaving behind overloaded functions with the old signatures so that all the existing logic can continue to work unmodified.
|
|
* Test to check fix
|
|
The HLSL language has keywords with very common names like `triangle`, and Slang doesn't want to preclude users from using such names for their variables/functions/etc.
In addition, Slang adds new keywords on top of HLSL (like `extension`) and we don't want those to prevent us from compiling existing code.
As a result, almost all keywords in Slang are contextual keywords, and they can be shadowed by user varaibles.
The down-side to making all keywords contextual is that in a case like this:
```
int test() { return triangle; }
```
The identifier `triangle` is *not* undefined as far as lookup (it is defined as a modifier keyword), so the existing "undefined identifier" logic gets bypassed, and instead we ran into an internal compiler error trying to construct an expression that refers to a modifier keyword.
Fortunately, the internal compiler error in that case was overkill, and the compiler already had defensive logic to produce an expression with an error type if it couldn't figure out what the type of a declaration reference should be.
The main fix here is thus to emit an "undefined identifier" error instead of an internal compiler error at the point where we see an attempt to reference a declaration that shouldn't be available in an expression context.
In order to improve the quality of the diagnostic, the code for constructing declaration references was updated to pass along a source location to be used in error messages.
|
|
* WIP with vector float test.
* vector-float test working.
* Fixed remaing tests broken with init changes.
* Improve 64bit-type-support.md
* Disable tests broken on CI system for Dx.
* WIP: Make type available for comparison.
* Moved type conversion into TypeTextUtil.
* Add text/type conversions from DownstreamCompiler to TypeTextUtil.
* Allow compaison taking into account type.
* Removed quantize in vector-float.slang test.
|
|
* Split out binding writing.
* Pass in the entry type.
* Take into account output type with -output-using-type
Added GPULikeBindRoot
Added dxbc-double-problem test.
* Add the dxbc-double-problem test.
|
|
The `TEST_INPUT` facility allows textual Slang test cases to provide two kinds of information to the `render-test` tool:
1. Information on what shader inputs exist
2. Information on what values/objects to bind into those shader inputs
Under the first category of information, there exists supporting for attaching a `dxbinding(...)` annotation to a `TEST_INPUT` which seemingly indicates what HLSL `register` the input uses. There is a similar `glbinding(...)` annotation, used for OpenGL and Vulkan.
It turns out that these annotations were, in practice, completely ignored and had no bearing on how `render-test` allocates or bindings graphics API objects. There was some amount of code attempting to validate that explicit registers/bindings were being set appropriately, but the actual values were being ignored.
The visible consequence of the `dxbinding` and `glbinding` annotations being ignored is issue #1036: the order of `TEST_INPUT` lines was *de facto* determining the registers/bindings that were being used by `render-test`.
This change simply removes the placebo features and strips things down to what is implemented in practice: the `TEST_INPUT` lines do not need target-API-specific binding/register numbers, because their order in the file implicitly defines them.
I added logic to the parsing of `TEST_INPUT` lines to make sure I got an error message on any leftover annotations, and went ahead and systematicaly deleted all of the placebo annotations from our test cases.
If we decide to make `TEST_INPUT` lines *not* depend on order of declaration in the future, we can build it up as a new and better considered feature.
The main alternative I considered was to keep the annotations in place, and change `render-test` and the `gfx` abstraction layer to properly respect them, but that path actually creates much more opportunity for breakage (since every single test case would suddenly be specifying its root signature / pipeline layout via a different path using data that has never been tested). The approach in this change has the benefit of giving me high confidence that all the test cases continue to work just as they had before.
|
|
glsl could not be coerced. (#1117)
|
|
* Strip IR after front-end steps are done
The main feature of this change is to unconditonally strip out the `IRHighLevelDeclDecoration`s in an IR module once the "mandatory" IR passes in the front end have run. This ensures that later IR passes (e.g., code emission) *cannot* rely on AST-level information to get their job done.
Since I was already writing a pass to remove some instructions at the end of the front-end passes, I went ahead and also made the `-obfuscate` flag apply to the front-end IR generation by causing it to strip `IRNameHintDecoration`s while it is doing the other stripping. With this, the main identifying information left in IR modules (other than semantics and entry-point names) is mangled name strings for imported/exported symbols.
A few other things got changes along the way:
* Removed the `.expected` file for one of the tests, where that file seemingly shouldn't have been checked in at all.
* Updated the signature of the DCE pass both so that it doesn't require a back-end compile request (it wasn't using it anyway), and so that it takes some options to decide whether to keep symbols marked `[export(...)]` alive (the front-end wants to keep these, while back-end passes currently need to be able to eliminate them).
* Moved the `obfuscateCode` flag from the back-end compile request to the base class shared between front- and back-end requests, and updated the options and repro logic to set both as needed. An obvious improvement in the future would be to have the front- and back-end requests share these settings by referencing a single common object in the end-to-end case, rather than each having their own copy.
* Removed logic that was keeping layout instructions alive in DCE, even if they weren't used. This seems to have been a vestige of an intermediate step between AST and IR layout.
* fixup: add the new files
|
|
compiling shaders that used texture2DMS Load() operations
|
|
This appears to be a regression introduced in #1001, and missed because none of our existing tests covered `static const` arrays on the GLSL/SPIR-V targets.
The basic problem is that we cannot output a `static const` definition in GLSL because `static` is a reserved word and not a keyword. Instead for GLSL we just want a `const` array. This change makes the emission of `static` for global-scope constants key on the target language for code generation, and only emit it for HLSL, C, and C++.
This change also adds a test case specifically for running Slang input that has a `static const` array on the Vulkan target.
|
|
* Convert bitwise Or & And to logical operations on scalar bools
* Test bitwise operations on scalar bools
|
|
Fixes #941
The GLSL we were emitting for unbounded-size arrays was the obvious:
```hlsl
// This HLSL:
Texture2D t[];
```
```glsl
// ... becomes this GLSL:
texture2D t[];
```
Unfortunately, the legacy GLSL behavior for an array without a declared size is what is called an "implicitly-sized" array, which means that it is assumed to actually have a fixed size, which is determined by the maximum integer constant value used to index into it (and only integer constants are allowed to be used when indexing into it).
Users hadn't noticed the issue for a while, because most of our users who rely on unbounded-size arrays were also using the HLSL `NonUniformResourceIndex` function:
```hlsl
float4 v = t[NonUniformResourceIndex(idx)].Sample(...);
```
When mapping such code to GLSL we use the `nonuniformEXT` qualifier added by the `GL_EXT_nonuniform_qualifier` extension, and it turns out that a secondary feature of that extension is that it changes the GLSL language semantics for arrays (of resources) with an unspecified size, so that they instead behave like we want. So users were happy and we were blissfully ignorant of the lurking issue.
The problem is that as soon as a user neglects to use `NonUniformResourceIndex` (perhaps because an index really is uniform):
```hlsl
cbuffer C { uint definitelyUniform; }
...
float4 v = t[definitelyUniform].Sample(...);
```
Now the code we emit doesn't need `nonuniformEXT` so it doesn't enable `GL_EXT_nonuniform_qualifier` and the declaration of `t` now falls under the "implicitly-sized" array rules, and thus the code fails because `definitelyUniform` is being used as an index but is *not* an integer constant.
The fix is pretty simple: when emitting a declaration of a global shader parameter to GLSL, we check if it is an unbounded-size array of resources and, if so, enable the `GL_EXT_nonuniform_qualifier` extension.
We don't need any clever handling to deal with resource parameters nested in `struct` types or in entry-point parameter lists, etc., because previous IR passes will have split up complex types and moved everything to the global scope already.
|
|
* Allow plugging in types with resources for interface parameters
The key feature enabled by this change is that you can take a shader declared with interface-type parameters:
```hlsl
ConstantBuffer<ILight> gLight;
float4 myShader(IMaterial material, ...)
{ ... }
```
and specialize its interface-type parameters to concrete type that can contain resources like textures, samplers, etc.
The hard part of doing this layout is that we need to support signatures that include a mix of interface and non-interface types. Imagine this contrived example:
```hlsl
float4 myShader(
Texture2D diffuseMap,
ILight light,
Texture2D specularMap)
{ ... }
```
We end up wanting `diffuseMap` to get `register(t0)` and `specularMap` to get `register(t1)`, so that they have the same location no matter what we plug in for `light`.
But if we plug in a concrete type for `light` that needs a texture register, we need to allocate it *somewhere*.
We handle this by having the `TypeLayout` for `light` come back with a "primary" type layout that doesn't have any texture registers, but with a "pending" type layout that includes the texture register requirements of whatever concrete type we plug in.
This split between "primary" and "pending" layout then needs to work its way up the hierarchy, so that an aggregate `struct` type with a mix of interface and non-interface fields (recursively), needs to compute an aggregate "primary type layout" and an aggregate "pending type layout," and then each field needs to be able to compute its offset in the primary/pending layout of the aggregate.
A large chunk of the work in this PR is then just implementing the split between primary and pending data, and ensuring that layouts are computed appropriately.
The next catch is that when a "parameter group" (either a parameter block or constant buffer) contains one or more values of interface type, then we can allow the parameter group to "mask" some of the resource usage of the concrete types we plug in, but others "bleed through."
For example, if we have:
```hlsl
struct MyStuff { float3 color; ILight light; }
ConstantBuffer<MyStuff> myStuff;
struct SpotLight { float3 position; Texture2D shadowMap; }
``
If we plug in the `SpotLight` type for `myStuff.light`, then the `float3` data for the light can be "masked" by the fact that we have a constant buffer (we can just allocate the `float3` `position` right after `color`), but the `Texture2D` needed for `shadowMap` needs to "bleed through" and become "pending" data for the `myStuff` shader parameter.
Adding support for that detail more or less required a full rewrite of the logic for allocating parameter group type layouts.
The next detail is that when we go to legalize a declaration like the `myStuff` buffer, we will end up with something like:
```hlsl
struct MyStuff_stripped { float3 color; }
struct Wrapped
{
MyStuff_stripped primary;
SpotLight pending;
}
ConstantBuffer<Wrapped> myStuff;
```
This "wrapped" version of the buffer type more accurately reflects the layout we need/want for the uniform/ordinary data, but in order to further legalize it and pull out the resource-type fields like `shadowMap` we need to have accurate layout information, and the problem is that layout information for the original buffer can't apply to this new "wrapped" buffer.
The last major piece of this change is logic that runs during existential type legalization to compute new layouts for "wrapped" buffers like these that embeds correct offset/binding/register information for any resources nested inside them. A key challenge in that code is that existential legalization needs to erase any "pending" data from the program entirely, so that offset information that used to be relatie to the "pending" part of a surrounding type now needs to be relative to the primary part.
The work here may not be 100% complete for all scenarios, but it does well enough on the new and existing tests that I want to checkpoint it. Note that a few other tests have had their output changed, but in all cases I've reviewed the diffs and determined that the change in observable behavior is consistent with what we intened Slang's behavior to be.
Note that there is still one major piece of support for interface-type parameters that is missing here, and which might force us to revisit some of the decisions in this code: we don't properly support user-defined `struct` types with interface-type fields.
* fixup: typos
|
|
|
|
* * Handle ! for bool vector in glsl
* Handle operators that have a boolean return value
* || or && take bool
* * Add comment in bool-op.slang test about doing || or && on vector types not supported for GLSL targets
|
|
* Add support for glsl inversesqrt intrinsic
* fixup for test failure
|
|
* Fix texture2d-gather test failure on dx12.
* Fix tab
|
|
* Add diagnostic for vk::binding failure.
* Add test for vk::binding failure.
* Add the expected output for glsl-layout-define.hlsl
* * Copy over initialize expr if available when validating unchecked
* Fix unloop - because now it always has one parameter (when before it could have none)
* Split vk::binding and layout tests with invalid parameters
* Removed the diagnostic for 2 ints expected
* Added vk::binding that doesn't specify set in vk-bindings.slang
* * Fix typo
* Improve comments.
|
|
* First pass test to see if GatherRed works.
* Add support for generating R_Float32 textures.
* Set default texture format.
* * Alter the texture2d-gather to work with a R_Float32 texture
* Add support for scalar Texture2d types with GatherXXX in stdlib
* Remove some left over commented out test code from texture2d-gather.hlsl
|
|
* Add intrinsic for StructuredBuffer.Load
|
|
|
|
Fixes #841
This reverts a small change made in #815 that seemed innocent at the time: we stopped tracking an explicit `Stage` to go with every `VarLayout` that is part of an entry-point varying parameter, and instead only associated the stage with the top-level parameter. That change ended up breaking the logic to emit the `flat` modifier automatically for integer type fragment-shader inputs for GLSL, but we didn't have a regression test to catch that case.
This change adds a regression test to cover this case, and adds the small number of lines that were removed from `parameter-binding.cpp`.
A few other test outputs had to be updated for the change (these are outputs that were changed in #815 for the same reason).
|
|
* * Make vector comparisons out correct functions on glsl
* Test for vector comparisons
* Typo fixes
* Glsl vector comparisons use functions.
* Added a coercion test.
* Do checking for the SV_DispatchThreadId type to see if it appears valid.
* Fix typo
* Make glsl do type conversion for SV_DispatchThreadID parameter.
* Fix glsl to match func-resource-param-array with changes to how SV_DispatchThreadID changes.
|
|
* * Make vector comparisons out correct functions on glsl
* Test for vector comparisons
* Typo fixes
* Glsl vector comparisons use functions.
* Added a coercion test.
|
|
* Allow entry points to have explicit generic parameters
Prior to this change, the Slang implementation required users to use global `type_param` declarations in order to specialize a full shader. For example:
```hlsl
type_param L : ILight;
ParameterBlock<L> gLight;
[shader("fragment")]
float4 fs(...)
{ ... gLight.doSomething() ... }
```
With this change we can rewrite code like the above using explicit generics, plus the ability to have `uniform` entry-point parameters:
```hlsl
[shader("fragment")]
float4 fs<L : ILight>(
uniform ParameterBlock<L> light,
...)
{ ... light.doSomething() ... }
```
Having this support in place should make it possible for us to eliminate global generic type parameters and the complications they cause (both at a conceptual and implementation level).
The most central and visible piece of the change is that `EntryPointRequest` now holds a `DeclRef<FuncDecl>` instead of just ` RefPtr<FuncDecl>`, which allows it to refer to a specialization of a generic function.
Various places in the code that refer to the `EntryPointRequest::decl` member now use a `getFuncDecl()` or `getFuncDeclRef()` method as appropriate (see `compiler.h`).
In order to fill in the new data, the `findAndValidateEntryPoint` function has been greaterly overhauled.
The changes to its operation include:
* The by-name lookup step for the entry point function has been adapted to accept either a function or a generic function.
* The generic argument strings provided by API or command line are no longer parsed all the way to `Type`s, but instead just to `Expr`s in the first pass.
* There are now two cases for checking the global generic arguments against their matching parameters. The first case is the new one, where we plug the generic argument `Expr`s into the explicit generic parameters of an entry point (that case re-uses existing semantic checking logic). The second case is the pre-existing code for dealing with global generic type arguments.
The `lower-to-ir.cpp` logic for hadling entry points then had to be extended. Making it deal with a full `DeclRef` instead of just a `Decl` was the easy part (just call `emitDeclRef` instead of `ensureDecl`).
The more interesting bits were:
* We need to carefully add the `IREntryPointDecoration` to the nested function and not the generic in the case where we have a generic entry point. There is a handy `getResolvedInstForDecorations` that can extract the return value for an IR generic so that we can decorate the right hting.
* We need to make sure that in the case where we emit a `specialize` instruction (which normally wouldn't get a linkage decoration), we attach an `[export(...)]` decoration to it with the mangled name of the decl-ref, so that it can be found during the linking step.
The IR linking step is then slightly more complicated because the mangled entry point name could either refer directly to an `IRFunc` or to a `specialize` instruction for a generic entry point. The logic was refactored to first clone the entry point symbol without concern for which case it is (the old code was specific to functions), and then *if* the result is a `specialize` instruction, we attempt to run generic specialization on-demand.
That on-demand specialization is a bit of a kludge, but it deals with the fact that all the downstream passing only expect to see an `IRFunc`. A future cleanup might try to split out that specialization step into its own pass, which ends up being a limited form of the specialization pass.
Since I was already having to touch a lot of the code around IR linking, I went ahead and refactored the signature of the operations. I eliminated the need for the caller to create, pass in, and then destroy an `IRSpecializationState` (really an IR *linking* state), and replaced it with a structure local to the pass (that data structure was a remnant of an older approach in the compiler), and then also renamed the main operation to `linkIR` to reflect what it is doing in our conceptual flow.
Smaller changes made along the way include:
* Refactored `visitGenericAppExpr` to create a subroutine `checkGenericAppWithCheckedArgs` so that it can be used by the entry-point validation logic described above).
* Refactored the declarations around the IR passes in `emitEntryPoint()` (`emit.cpp`), to show that things are more self-contained than they used to be (e.g., that the `TypeLegalizationContext` is now only needed by one pass).
* Refactored the generic specialization code so that there is a stand-along free function that can perform specialization on a `specialize` instruction without all the other context being required. This is only to support the limited specialization that needs to be done as part of linking.
* Updated the `global-type-param.slang` test to actually test entry-point generic parameters. In a later pass we can/should rework all the tests/examples for global type parameters over to use explicit entry-point generic parameters (at which point we should rename the tests as well). For now I am leaving thigns with just one test case, with the expectation that bugs will be found and ironed out as we expand to more tests.
* fixup
* Fixup: don't leave entry-point decorations on stuff we don't want to keep
The IR `[entryPoint]` decoration is effectively a "keep this alive" decoration, which means that attaching it to something we don't intend to keep around can lead to Bad Things.
The approach to generic entry points was attaching `[entryPoint]` to the underlying `IRFunc` because that seemed to make sense, but that meant that the `specialize` instruction at global scope scould instantiate that generic and then keep it alive, even if the resulting function wouldn't be valid according to the language rules.
As a quick fix, I'm attaching `[entryPoint]` to the `specialize` instruction instead in such cases, and then re-attaching it to the result of explicit specialization during linking.
* Port most of remaining test and rename global type parameters
This change ports as many as possible of the existing tests for global type parameters over to use entry-point generic parameters instead. For the most part this is a mechanical change.
A few test cases remain using global generic parameters, as does the `model-viewer` example application.
The reason for this is that the shaders have either or both the following features:
* A vertex and fragment shader that can/shold agree on their parameters
* A type declaration (e.g., a `struct`) that is dependent on one of the generic type parameters
In these cases, it would really only make sense to switch to explicit parameters once we support shader entry points nested inside of a `struct` type, so that we can use an outer generic `struct` as a mechanism to scope the entry points and other type-dependent declrations.
Since global-scope type parameters need to persist for at least a bit longer, I went ahead and renamed all the use sites over to use `type_param` for consistency.
|
|
|
|
There was a bug in the logic for emitting initial IR, such that it was neglecting to emit "methods" (member functions) unless they were also referenced by a non-member (global) function, or were needed to satisfy an interface requirement. This would only matter for `import`ed modules, since for non-`import`ed code, anything relevant would be referenced by the entry point so that the problem would never surface.
This change fixes the underlying problem by adding a step to the IR lowering pass called `ensureAllDeclsRec` that makes sure that not only global-scope declarations, but also anything nested under a `struct` type gets emitted to the initial IR module.
There are also a few unrelated fixes in this PR, which are things I ran into while making the fix:
* Deleted support for the (long gone) `IRDeclRef` type in our `slang.natvis` file
* Added support for visualizing the value of IR string and integer literals when they appear in the debugger
* Fixed IR dumping logic to not skip emitting `struct` and `interface` instructions. Switching those to inherit from `IRType` accidentally affected how they get printed in IR dumps by default.
* Fixed up the IR linking logic so that it correctly takes `[export]` decorations into account, so that an exported definition will always be taken over any other (unless the latter is more specialized for the target). I initially implemented this in an attempt to fix the original issue, but found it wasn't a fix for the root cause. It is still a better approach than what was implemented previously, so I'm leaving it in place.
|
|
Fixes #775
It was reported (in #775) that Slang doesn't handle initializer-list syntax when initializing matrix variables. When starting on a fix for that it became apparent that the time was right to fix two broad issues in the compiler's current handling of `{}`-enclosed initializer lists.
The first issue was that the front-end checking of initializer lists wasn't handling the C-style behavior where an initializer list can either contain nested `{}`-enclosed lists for sub-arrays/-structures, or directly contain "leaf" values for initializing those aggregates. For example, the following two variable declarations ought to be equivalent:
```hlsl
int4 a[] = { {1, 2, 3, 4}, {5, 6, 7, 8} };
int4 b[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
```
Getting this distinction right is important because we want to support initializing a matrix either from a list of vectors for its rows, or a list of scalars for its elements (in row-major order).
The front-end semantic checking logic for initializer lists was revamped so that it conceptually tries to "read" an expression of a desired type from the initializer list, and decides at each step whether to consume a single expression by coercing it to the desired type, or to recursively read multiple sub-values to construct the type as an aggregate. The logic for deciding between direct vs aggregate initialization could potentially use some tweaking, but luckily it should always handle the case where users introduce explicit `{}`-enclosed sub-lists to make their intention clear, so that existing Slang code should continue to work as before.
The second issue was that initializers without the expected number of elements weren't implemented in code generation, so they would lead to internal compiler errors. This change revamps the codegen logic for initializer lists so that it can synthesize default values for fields/elements that were left out during initialization. This includes an attempt to support default initialization of `struct` fields based on explicitly written initialization expressions.
|
|
The IR linking logic was recently rewritten to use the (optional) `IRLinkageDecoration`s instead of assuming `IRGlobalVals` always have a mangled name field, and in that process a bug seems to have crept in where in the case that an instruction that would usually quality as a "global value" does *not* have linkage, we were failing to register the instruction we create in the output module as a replacement for the original instruction.
This problem affects `static` variables inside of functions, leading to them potentially getting emitted multiple times.
|
|
The logic in `check.cpp` for declaration checking is very messy and needs to be re-written, but in the interim we need to be careful to avoid any cases where a declaration, or some piece of it, gets redundantly checked multiple times.
The way the logic had been working, the different "cases" in an `enum` type were being checked twice, and that meant that any initialization expression for a case would be type-checked the first time (potentially leading to a new AST) and then the checked AST would be checked again. This created a problem if the first round of checking introduced any AST nodes that the checking logic would not expect to see (because the parser cannot possibly produce them).
The fix here is to follow the style of the other declaration checking cases, where checking is separated into two distinct phases (the "header" phase makes the declaration usable by others, while the "body" phase checks its implementation details for internal consistency).
This change includes a test case that produced an internal compiler error before, and compiles without error now.
|
|
* Change how buffers are emitted
This is a change with a lot of pieces, which can't always be separated out cleanly. I'm going to walk through them in what I hope is a logical order.
The main goal of this change was to allow arrays of structured buffers to translate to Vulkan. Consider two declarations of structured buffers in HLSL/Slang:
```hlsl
StructuredBuffer<X> single;
StructuredBuffer<Y> multiple[10];
```
The current translation logic was handling `single` by translating it into an *unnamed* GLSL `buffer` block like:
```glsl
layout(std430)
buffer _S1
{
X single[];
};
```
That syntax allows an expression like `single[i]` in Slang to be translated simply as `single[i]` in GLSL.
But that naive translating doesn't work for `multiple`, since we need to declare a array of blocks in GLSL, which requires giving the whole thing a name:
```glsl
layout(std430)
buffer _S2
{
Y _data[];
} multiple[10];
```
Now a reference to `multiple[i][j]` in Slang needs to become `multiple[i]._data[j]` in GLSL.
To avoid having way too many special cases around single structured buffers vs. arrays, it makes sense to allows emit things in the latter form, so that we instead lower `single` as:
```glsl
layout(std430)
buffer _S1
{
X _data[];
} single;
```
So that now a reference to `single[i]` becomes `single._data[i]` in GLSL.
Most of that can be handled in the standard library translation of the structured buffer indexing operations.
The only wrinkle there is that there were some *old* special-case instructions in the IR intended to handle buffer load/store operations (these were added back when I was trying to keep the "VM" path working). These aren't really needed to have structured-buffer operations work; they can be handled as ordinary functions as far as the stdlib is concerned. I removed the old instructions.
Along the way, it became clear that a few other cases follow the same pattern. Byte-addressed buffers are an obvious case. We were lowering HLSL/Slang:
```hlsl
ByteAddressBuffer b;
...
uint x = b.Load(0);
```
to GLSL like:
```glsl
layout(std430)
buffer _S1
{
uint b[];
};
...
uint x = b[0];
```
That logic would fail for arrays the same way that the structured buffer case was failing. The fix is the same: use named `buffer` blocks and then introduce an explicit `_data` field:
```glsl
layout(std430)
buffer _S1
{
uint _data[];
} b;
...
uint x = b._data[0];
```
Just like with structured buffers, all of the VK translation for operations on byte-addressed buffers can be implemented directly in teh stdlib, so once the emit logic was changed it was just a matter of adding `._data` to a bunch of VK tranlsations.
It turns out that arrays of constant buffers have more or less the same problem, and furthermore we have some problems with any code that directly uses the modern HLSL `ConstantBuffer<T>` type.
Note: the emit logic around constant buffers sometimes refers to "parameter groups" because that is being used in the compiler as a catch-all term for constant buffers, texture buffers, and parameter blocks.
The existing code was going out of its way to reproduce the way that constant buffer declarations are implicitly referenced in HLSL:
```hlsl
cbuffer C { float f; }
...
float tmp = f; // No reference to `C` here
```
This can be seen in the emit logic with the `isDerefBaseImplicit` function, which is used to take the internal IR representation for a reference to `f` (which is closer to the expression `(*C).f` or `C->f`) and leave off any reference to `C` so that we emit just `f`.
That kind of logic just flat out doesn't work in some important cases. Arrays of constant buffers are a clear one:
```hlsl
ConstantBuffer<X> cbArray[3];
...
X x = cbArray[0];
```
There is no way to translate that to an ordinary `cbuffer` declaration at all. The same problem can be created without arrays, though:
```hlsl
ConstantBuffer<X> singleCB;
...
X x = singleCB;
```
The current strategy for translating constant buffers was translating `singleCB` into a `cbuffer` declaration that reproduced the fields of `X` as its members, which just wouldn't work:
```hlsl
cbuffer singleCB
{
float f; // field of `X`
}
...
X x = singleCB; // ERROR: there is nothing named `singleCB` in this HLSL
```
The new strategy is more consistent. We still generate a `cbuffer` declaration for a single constant buffer, but we always give it a single field of the chosen element type:
```hlsl
cbuffer singleCB
{
X singleCB;
}
...
X x = singleCB; // this works fine!
```
And in the array case we generate code that uses the explicit `ConstantBuffer<T>` type:
```hlsl
ConstantBuffer<X> cbArray[3];
...
X x = cbArray[0];
```
The GLSL output is more complicated because unlike with HLSL there is no implicit conversion from a uniform block to its element type (there is no notion of an element type). The array case thus needs a `_data` field similar to what we do for structured buffers:
```glsl
layout(std140)
uniform _S3
{
X _data;
} cbArray[3];
...
X x = cbArray[0]._data;
```
And then the non-array case needs to have a similar `_data` field for consistency:
```glsl
layout(std140)
uniform _S1
{
X _data;
} singleCB;
...
X x = singleCB._data;
```
This is handled by inserting the necessary reference to `_data` whenever we dereference a constant buffer, either as part of a load instruction (loading from the whole CB as a pointer), or an `IRFieldAddress` instruction which forms a pointer into the CB (e.g., `&(singleCB->f)` becomes `singleCB._data.f`).
The current emit logic handles `ParameterBlock<X>` differently from `ConstantBuffer<X>`, but really only to allow parameter blocks to be explicitly named in the output, while constant buffers were left implicit by default. Thus the only difference was a legacy one (from back when trying to exactly reproduce the HLSL text we got as input was considered an important goal), and the new approach to emitting constant buffers would get rid of it.
I removed the separate logic for emitting `ParameterBlock<X>` and just let the handling for constant buffers deal with it.
Note that any resource types inside of a `ParameterBlock<X>` would have been moved out as part of legalization, so that a parameter block is 100% equivalent to a constant buffer when it comes time to emit code.
Unsurprisingly, changing the way we generate HLSL and GLSL output for all these buffer types meant that any tests that were directly comparing the output of `slangc` against `fxc`, `dxc`, or `glslang` broke.
The basic approach to fixing the breakage in GLSL tests was to update the GLSL baseline to reflect the new output startegy. In some cases I used macros to name the various `_S<digits>` temporaries so that future renaming will hopefully be easier (it would be great if we auto-generated temporary names with a bit more context). There was one GLSL test (`tests/bugs/vk-structured-buffer-binding`) that was using raw GLSL expected output, and this was changed to use a GLSL baseline to generate SPIR-V for comparison.
For HLSL tests we were sometimes running the same input file through `slangc` and `fxc`/`dxc`, and in these cases I macro-ized the various `cbuffer` declarations to generate different declarations depending on the compiler.
I completely dropped the tests coming from the D3D SDK because they aren't providing much coverage, and updating them would change them so far from the original code that the purported benefit (using a body of existing shaders) would be lost.
I also dropped the explicit matrix layout qualifiers in the `matrix-layout` test because the new output strategy breaks those for GLSL (you can't put matrix layout qualifiers on `struct` fields, and now the body of every constant buffer is inside a `struct`). This isn't as big of a loss as it seems, because our handling of those qualifiers wasn't really right to begin with. Slang users should only be setting the matrix layout mode globally (and we should probably switch to error out on the explicit qualifiers for now).
The other thing that got dropped is tests involving `packoffset` modifiers.
Slang already warns that it doesn't support these, and the way they were used in the test cases is actually misleading. For the binding/layout-related tests, the goal was to show that Slang reproduces the same layout as fxc, in which case explicitly enforcing a layout via `packoffset` seems like cheating (are we sure we enforced the layout fxc would have produced?). The real reason was that Slang used to emit explicit `packoffset` on *every* field of a `cbuffer` it would output, because of an `fxc` bug where you couldn't use `register` on textures/samplers declared inside a `cbuffer` unless *every* field in the `cbuffer` used a `register` or `packoffset` modifier. Slang hasn't required that behavior in a while because it now splits textures and samplers, and the one test case where we needed `packoffset` to work around the `fxc` bug in the baseline HLSL has been macro-ified even more to work around the bug.
The amount of churn in the test cases is unfortunate, but it continues to point at the weakness of any testing strategy that checks for exact equivalent between Slang's output and that of other compilers. We need to keep working to replace these tests with better alternatives.
In `check.cpp` there is logic to perform implicit dereferencing, so that if you write `obj.f` where `obj` is a `ConstantBuffer<X>` (or some other "pointer-like" type) and `f` is a field in `X`, then this effectively translates as `(*obj).f`. That is, we dereference the value of type `ConstantBuffer<X>` to get a value of type `X`, and then refer to the field of the `X` value.
There was a problem where the logic to insert that kind of implicit dereference operation was using a reference (`auto& type = ...`) for the type of the expression being dereferenced, and then clobbering it. This would mean that an expression of type `ConstantBuffer<X>` would have its type overwritten to be just `X` and then codegen would break later on.
I'm not sure how we haven't run into that before.
The `array-of-buffers` test case was added to confirm that we now support arrays of constant, structured, and byte-address buffers for both DXIL and SPIR-V output.
Okay, so that was a lot of stuff, but hopefully it is clear how this all works to make the output of the compiler more consistent and explicit, while also supporting the required new functionality.
* fixup: review feedback
|
|
* Fix output of binding of structured buffer on GLSL.
* Added test to check vk binding is coming thru.
* Fix closethit binding inconsistency.
|
