diff options
| author | Tim Foley <tfoleyNV@users.noreply.github.com> | 2020-07-31 16:33:30 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2020-07-31 16:33:30 -0700 |
| commit | bb42514acc03a476a93ae82fa0cc9d02dce86239 (patch) | |
| tree | cd7340c16d7657a4a47ad5195f87113a4519db42 /source | |
| parent | 011a743668e7cd0b7cf97d27e3bed7d519794aeb (diff) | |
Fix issues arising around DXR 1.1 RayQuery usage (#1468)
This change includes a few different fixes for issues that arose in a user shader that made use of DXR 1.1.
The existing solution we had for handling the DXR 1.1 `RayQuery` type relied on the fact that a declaration like:
```hlsl
RayQuery<0> myRayQuery;
```
Looks like an undefined variable to existing Slang, while to dxc it is a variable declaration that runs an implicit default constructor (sneaking a bit of C++ into HLSL, but only in a way the standard library can use).
Slang was getting away with the fact that this maps to an undefined variable because it turns out that our emit logic would output the exact same declaration for an undefined value (since declaring a variable without initializing it is the simplest way to get an undefined value of a given type in a C-like language).
The main bug that arose here was that if the `RayQuery<...>`-typed variable was declared under control flow, then the `undefined` instructions introduced by our SSA pass would actually get inserted into the wrong block. Basically, when a block was trying to read a variable, and there was no preceding `store` to that variable in the block, we'd start looking for incoming values from its predecessor block(s). In the case where the variable *never* gets stored to, this search would eventually reach the first block of the function, where we'd realize the value must be `undefined`. The result was that we might insert an `undefined` instruction of some `T` into the first block of a function, but the type `T` might be the result of a lookup operation performed later in that function. This ends up creating a use of `T` that isn't dominated by the definition, which violates the SSA property. This violation of the SSA properties lead to us generating incorrect code in a later pass that deals with scoping differences between SSA form and our structured output statements; that code would end up creating a local variable to hold a *type* instead of a value.
The main fix is in `slang-ir-ssa.cpp`, where we catch the case of trying to read a variable in the block that declares it, if there we no preceding `store`s. We simply insert an `undefined` instruction before the first such read and write that out as the value of the variable to be used for subsequent instructions (up to the next `store`). This fixes the SSA dominance property for the `undefined` values that get introduced and thus technically fixes the output code for the user shader.
A secondary issue is that it is kind of gross to be relying on the behavior of `undefined` instructions in the IR for the semantics of an important standard library type like `RayQuery<...>`. A preceding change already added basic support for Slang to run default initializers (declared as `__init()`) on variables that are declared without an initial-value expression. This change adds such a default initializer to `RayQuery<...>` and maps it to a dedicated IR instruction that is intended to represent the idea of running a C++-style default constructor to produce a value. It turns out that the code we need to emit in that cse is identical to what we currently emit for `undefined` instructions, so that is helpful.
A tertiary issue is that when trying to run the user shader in debug mode, I ran into an assertion because our type layout logic for reflection had never dealt with the issue of user-defined `enum` types being used in constant buffers or other memory that needs layout. I added a quick fix that lays out any `enum` types as their "tag type" (which defaults to `int`).
Unfortunately, there is no easy way to check in a regression test for the user issue, because official `dxcompiler` versions with support for DXR 1.1 are not yet released (at least as of last time I checked).
Diffstat (limited to 'source')
| -rw-r--r-- | source/slang/hlsl.meta.slang | 5 | ||||
| -rw-r--r-- | source/slang/slang-emit-c-like.cpp | 2 | ||||
| -rw-r--r-- | source/slang/slang-ir-inst-defs.h | 6 | ||||
| -rw-r--r-- | source/slang/slang-ir-ssa.cpp | 13 | ||||
| -rw-r--r-- | source/slang/slang-ir.cpp | 1 | ||||
| -rw-r--r-- | source/slang/slang-type-layout.cpp | 11 |
6 files changed, 38 insertions, 0 deletions
diff --git a/source/slang/hlsl.meta.slang b/source/slang/hlsl.meta.slang index 8a7c3a010..69fc6bea0 100644 --- a/source/slang/hlsl.meta.slang +++ b/source/slang/hlsl.meta.slang @@ -4041,6 +4041,11 @@ static const CANDIDATE_TYPE CANDIDATE_PROCEDURAL_PRIMITIVE = 1; __target_intrinsic(hlsl, RayQuery) struct RayQuery <let rayFlags : RAY_FLAG = RAY_FLAG_NONE> { + // Initialize the query object in a "fresh" state. + // + __intrinsic_op($(kIROp_DefaultConstruct)) + __init(); + // Initialize a ray-tracing query. // // This method may be called on a "fresh" ray query, or diff --git a/source/slang/slang-emit-c-like.cpp b/source/slang/slang-emit-c-like.cpp index 4c6c89ef5..a7637a63e 100644 --- a/source/slang/slang-emit-c-like.cpp +++ b/source/slang/slang-emit-c-like.cpp @@ -2037,6 +2037,7 @@ void CLikeSourceEmitter::defaultEmitInstExpr(IRInst* inst, const EmitOpInfo& inO break; case kIROp_undefined: + case kIROp_DefaultConstruct: m_writer->emit(getName(inst)); break; @@ -2464,6 +2465,7 @@ void CLikeSourceEmitter::_emitInst(IRInst* inst) break; case kIROp_undefined: + case kIROp_DefaultConstruct: { auto type = inst->getDataType(); emitType(type, getName(inst)); diff --git a/source/slang/slang-ir-inst-defs.h b/source/slang/slang-ir-inst-defs.h index e01121d36..ca3ca6dae 100644 --- a/source/slang/slang-ir-inst-defs.h +++ b/source/slang/slang-ir-inst-defs.h @@ -216,6 +216,12 @@ INST_RANGE(Constant, BoolLit, StringLit) INST(undefined, undefined, 0, 0) +// A `defaultConstruct` operation creates an initialized +// value of the result type, and can only be used for types +// where default construction is a meaningful thing to do. +// +INST(DefaultConstruct, defaultConstruct, 0, 0) + INST(Specialize, specialize, 2, 0) INST(lookup_interface_method, lookup_interface_method, 2, 0) INST(lookup_witness_table, lookup_witness_table, 2, 0) diff --git a/source/slang/slang-ir-ssa.cpp b/source/slang/slang-ir-ssa.cpp index 19f85eaa9..7c849e36a 100644 --- a/source/slang/slang-ir-ssa.cpp +++ b/source/slang/slang-ir-ssa.cpp @@ -776,6 +776,19 @@ IRInst* readVar( return val; } + if( blockInfo->block == var->parent ) + { + // If this is the block that actually *introduces* + // the variable, then there is no reason to keep + // searching, because its value cannot have been + // established in a predecessor block. + // + auto type = var->getDataType()->getValueType(); + val = blockInfo->builder.emitUndefined(type); + writeVar(context, blockInfo, var, val); + return val; + } + // Otherwise we need to try to non-trivial/recursive // case of lookup. return readVarRec(context, blockInfo, var); diff --git a/source/slang/slang-ir.cpp b/source/slang/slang-ir.cpp index a5e0de0c5..32034fec8 100644 --- a/source/slang/slang-ir.cpp +++ b/source/slang/slang-ir.cpp @@ -5171,6 +5171,7 @@ namespace Slang case kIROp_Nop: case kIROp_undefined: + case kIROp_DefaultConstruct: case kIROp_Specialize: case kIROp_lookup_interface_method: case kIROp_getAddr: diff --git a/source/slang/slang-type-layout.cpp b/source/slang/slang-type-layout.cpp index b01b275f9..8b4c80554 100644 --- a/source/slang/slang-type-layout.cpp +++ b/source/slang/slang-type-layout.cpp @@ -3641,6 +3641,17 @@ static TypeLayoutResult _createTypeLayout( return TypeLayoutResult(typeLayout, SimpleLayoutInfo()); } + else if( auto enumDeclRef = declRef.as<EnumDecl>() ) + { + // We lay out an enumeration type as its tag type. + // + // TODO: This code doesn't handle the case where we might + // have a generic `enum` (or an `enum` inside another generic + // type), and where the tag type of the `enum` depends on + // one or more of the generic parameters. + // + return _createTypeLayout(context, enumDeclRef.getDecl()->tagType); + } } else if (auto errorType = as<ErrorType>(type)) { |
