summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorTim Foley <tfoleyNV@users.noreply.github.com>2020-04-07 13:56:01 -0700
committerGitHub <noreply@github.com>2020-04-07 13:56:01 -0700
commitba232e44cbb016a4e7c111b42458394027369c5e (patch)
treec7d56cfcc5e4c94d0d82358663d82014904ce854
parentc5db04b8ba72d14d6bf2e20abf9c1b8d8466abc6 (diff)
Fix a bug around generic functions using DXR RayQuery (#1309)
The DXR `RayQuery` type is our first generic type defined in the stdlib that is marked as a target intrinsic but does *not* map to a custom `IRType` case. Instead, a reference to `RayQuery<T>` is encoded in the IR as an ordinary `specialize` instruction. Unfortunately, this doesn't play nice with the current specialization logic, which considered a `specialize` instruction to not represent a "fully specialized" instruction, which then inhibits specialization of generics/functions that use such an instruction. The fix here was to add more nuanced logic to the check for "fully specialized"-ness, so that it considers a `specialize` to already be fully specialized when the generic it applies to represents a target intrinsic. I also added a note that the whole notion of "fully specialized"-ness that we use isn't really the right thing for the specialization pass, and how we really ought to use a notion of what is or is not a "value." This change doesn't include a test because the only way to trigger the issue is using the DXR 1.1 `RayQuery` type, and that type is not supported in current release versions of DXC.
-rw-r--r--source/slang/slang-ir-specialize.cpp58
1 files changed, 57 insertions, 1 deletions
diff --git a/source/slang/slang-ir-specialize.cpp b/source/slang/slang-ir-specialize.cpp
index 6baf8fca5..d4f236138 100644
--- a/source/slang/slang-ir-specialize.cpp
+++ b/source/slang/slang-ir-specialize.cpp
@@ -366,6 +366,23 @@ struct SpecializationContext
void maybeMarkAsFullySpecialized(
IRInst* inst)
{
+ // TODO: The logic here is completely bogus and
+ // we need to revisit the notion of fully-specialized-ness
+ // to only involve things that are semantically *values*
+ // rather than computations/expressions.
+ //
+ // The rules should be something like:
+ //
+ // * Literals are values
+ // * Composite type constructors where all the operands are value are values
+ // * References to nominal types are values
+ // * Built-in types where all the operands are values are values
+ //
+ // The system for defining value-ness probably needs
+ // to combine with the system for deduplicating instructions,
+ // since values are an important class of instruction we want
+ // to deduplicate.
+
switch(inst->op)
{
default:
@@ -387,11 +404,50 @@ struct SpecializationContext
// Certain instructions cannot ever be considered
// fully specialized because they should never
// be substituted into a generic as its arguments.
- case kIROp_Specialize:
case kIROp_lookup_interface_method:
case kIROp_ExtractExistentialType:
case kIROp_BindExistentialsType:
break;
+
+ case kIROp_Specialize:
+ // The `specialize` instruction is a bit sepcial,
+ // because it is possible to have a `specialize`
+ // of a built-in type so that it never gets
+ // substituted for another type. (e.g., the specific
+ // case where this code path first showed up
+ // as necessary was `RayQuery<>`)
+ //
+ {
+ auto specialize = cast<IRSpecialize>(inst);
+ auto base = specialize->getBase();
+ if( auto generic = as<IRGeneric>(base) )
+ {
+ // If the thing being specialized can be resolved,
+ // *and* it is a target intrinsic, ...
+ //
+ if( auto result = findGenericReturnVal(generic) )
+ {
+ if( result->findDecoration<IRTargetIntrinsicDecoration>() )
+ {
+ // ... then we should consider the instruction as
+ // "fully specialized" in the same cases as for
+ // any ordinary instruciton.
+ //
+
+ if( areAllOperandsFullySpecialized(inst) )
+ {
+ markInstAsFullySpecialized(inst);
+ }
+ return;
+ }
+ }
+ }
+
+ // Otherwise, a `specialize` instruction falls into
+ // the case of instructions that should never be
+ // considered to be fully specialized.
+ }
+ break;
}
}