From 112caca00ba9bfd9e1051bb94969efa9e74c6c03 Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Thu, 8 Feb 2018 07:54:04 -0800 Subject: Falcor fixes (#402) * Re-define deprecated compile flags By including these flags in the header file, with a value of zero, we can allow some existing code to compile even after the major changes to the implementation. * The `SLANG_COMPILE_FLAG_NO_CHECKING` option will effectively be ignored, since checking is always enabled. * The `SLANG_COMPILE_FLAG_SPLIT_MIXED_TYPES` option will now act as if it is always enabled (and indeed some of the code has been relying on this flag being set always). * Make subscript operators writable for writable textures This even had a `TODO` comment saying that we needed to fix it, and now I'm seeing semantic checking failures because we didn't define these and so we find assignment to non l-values. * Fix definitions of any() and all() intrinsics These should always return a scalar `bool` value, but they were being defined wrong in two ways: 1. They were using their generic type parameter `T` in the return type 2. They were returning a vector in the vector case, and a matrix in the matrix case. This change just alters the return type to be `bool` in all cases. * Fix bug in SSA construction When eliminating a trivial phi node, it is possible that the phi is still recorded as the "latest" value for a local variable in its block. When later code queries that value from the block (which can happen whenever another block looks up a variable in its predecessors), it would get the old phi and not the replacement value. I simply added a loop that checks if the value we look up is a phi that got replaced, and then continues with the replacement value (which might itself be a phi...). A more advanced solution might try to get clever and have the map itself hold `IRUse` values so that we can replace them seamlessly. * Simplify IR control flow representation This change gets rid of various special-case operations for conditional and unconditional branches, and instead requires emit logic to recognize when a direct branch is targetting a `break` or `continue` label. The new approach here isn't perfect, but it seems beter than what we had before, because it can actually work in the presence of control-flow optimizations (including our current critical-edge-splitting step). * Load from groupshared isn't groupshared When loading from a `groupshared` variable, the resulting temporary shouldn't have the `groupshared` qualifier on it. This might eventually need to generalize to a better understanding of storage modifiers in the IR, but I don't really want to deal with that right now. * Don't emit references to typedefs in output code Now that we are using the IR for all codegen, we shouldn't be dealing with surface-level things like `typedef` declarations in the output code; just use the type that was being referred to in the first place. * Fix floating-point literal printing for IR The IR was calling `emit()` instead of `Emit()` (we really need to normalize our convention here), and was implicitly invoking a default constructor on `String` that takes a `double` (that constructor should really be marked `explicit`), and which doesn't meet our requirements for printing floating-point values. * Fix error when importing module that doesn't parse We already added a case to bail out if semantic checking fails, but neglected to add a case if there is an error during parsing of a module to be imported. Note: this logic doesn't correctly register the module as being loaded (but still in error), so users could see multiple error messages if there are multiple `import`s for the same module. * Improve error message for overload resolution failure - Drop debugging info from the candidate printing - Add cases to print `double` and `half` types properly * Fixup: switch loopTest to ifElse in expected IR output --- source/slang/ir-ssa.cpp | 49 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 11 deletions(-) (limited to 'source/slang/ir-ssa.cpp') diff --git a/source/slang/ir-ssa.cpp b/source/slang/ir-ssa.cpp index 366ecc279..8a2d201dd 100644 --- a/source/slang/ir-ssa.cpp +++ b/source/slang/ir-ssa.cpp @@ -28,9 +28,9 @@ struct PhiInfo : RefObject // order in which the predecessor blocks get enumerated. List operands; - // Did we end up removing this phi because it was determined - // to be trivial? - bool wasRemoved = false; + // If this phi ended up being removed as trivial, then + // this will be the value that we replaced it with. + IRValue* replacement = nullptr; }; // Information about a basic block that we generate/use @@ -270,12 +270,16 @@ IRValue* tryRemoveTrivialPhi( // of itself) with the unique non-phi value. phi->replaceUsesWith(same); - // We will mark the phi as removed in the `PhiInfo` structure, - // so that we can avoid adding it to the block later. - phiInfo->wasRemoved = true; + // Clear out the operands to the phi, since they won't + // actually get used in the program any more. + for( auto& u : phiInfo->operands ) + { + u.clear(); + } - // TODO: we need a way to record that this parameter - // is no longer to be used... + // We will record the value that was used to replace this + // phi, so that we can easily look it up later. + phiInfo->replacement = same; // TODO: need to recursively consider chances to simplify // other phi nodes now. @@ -476,6 +480,29 @@ IRValue* readVar( { // Hooray, we found a value to use, and we // can proceed without too many complications. + + // Well, let's check for one special case here, which + // is when the value we intend to use is a `phi` + // node that we ultimately decided to remove. + while( val->op == kIROp_Param ) + { + // The value is a parameter, but is it a phi? + IRParam* maybePhi = (IRParam*) val; + RefPtr phiInfo = nullptr; + if(!context->phiInfos.TryGetValue(maybePhi, phiInfo)) + break; + + // Okay, this is indeed a phi we are adding, but + // is it one that got replaced? + if(!phiInfo->replacement) + break; + + // The phi we want to use got replaced, so we + // had better use the replacement instead. + val = phiInfo->replacement; + } + + return val; } @@ -724,9 +751,9 @@ void constructSSA(ConstructSSAContext* context) for (auto phiInfo : blockInfo->phis) { - // If we eliminated this phi, then we had better not - // include it in the result. - if (phiInfo->wasRemoved) + // If we replaced this phi with another value, + // then we had better not include it in the result. + if (phiInfo->replacement) continue; // We should add the phi as an explicit parameter of -- cgit v1.2.3