From 13c6b69cc2601ff4764f095fb45ad9573d34ff2f Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Thu, 11 Oct 2018 11:17:40 -0700 Subject: Fix error when one constant is defined equal to another (#670) * Fix error when one constant is defined equal to another Fixes #666 When a user declares one constant (usually a `static const` variable) to be exactly equal to another by name: ```hlsl static const a = 999; static const b = a; ``` Then the IR-level representation of `b` is an `IRGlobalConstant` whose value expression is just a pointer to the definition of `a`. The logic in `emitIRGlobalConstantInitializer()` was trying to always call `emitIRInstExpr` to emit the value of the constant as an expression, but that function only handles complex/compound expressions and not the case of simple named values (e.g., constants like `a`). The intention is for code to call `emitIROperand()` instead, and let it decide whether to emit an expression or a named reference using its own decision-making. The `IRGlobalConstant` case really just wants to pass in the "mode" flag it uses to influence that decision-making, but shouldn't be working around it. This change just replaces the `emitIRInstExp()` call with `emitIROpernad()` and adds a test case to confirm that this fixes the reported problem. * Fixups for bugs in previous change The first problem was that certain instruction ops were being special-cased to opt out of "folding" into expressions *before* we make the universal check to always fold when inside an initializer for a global constant. The second problem is that the `emitIROperand()` logic was always putting expressions around sub-expressions, which breaks parsing when the sub-expression is an initializer list (`{...}`). This fixup is pretty much a hack, but will be something we can remove once we don't emit unncessary parentheses overall, which is a better fix. --- source/slang/emit.cpp | 64 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 15 deletions(-) (limited to 'source') diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp index 312e8c0b3..03c788ddb 100644 --- a/source/slang/emit.cpp +++ b/source/slang/emit.cpp @@ -2353,14 +2353,6 @@ struct EmitVisitor case kIROp_Param: return false; - // HACK: don't fold these in because we currently lower - // them to initializer lists, which aren't allowed in - // general expression contexts. - // - case kIROp_makeStruct: - case kIROp_makeArray: - return false; - // Always fold these in, because they are trivial // case kIROp_IntLit: @@ -2387,6 +2379,25 @@ struct EmitVisitor if (mode == IREmitMode::GlobalConstant) return true; + switch( inst->op ) + { + default: + break; + + // HACK: don't fold these in because we currently lower + // them to initializer lists, which aren't allowed in + // general expression contexts. + // + // Note: we are doing this check *after* the check for `GlobalConstant` + // mode, because otherwise we'd fail to emit initializer lists in + // the main place where we want/need them. + // + case kIROp_makeStruct: + case kIROp_makeArray: + return false; + + } + // Instructions with specific result *types* will usually // want to be folded in, because they aren't allowed as types // for temporary variables. @@ -2539,9 +2550,25 @@ struct EmitVisitor { if( shouldFoldIRInstIntoUseSites(ctx, inst, mode) ) { - emit("("); - emitIRInstExpr(ctx, inst, mode); - emit(")"); + // HACK: Don't emit parentheses for cases that will + // wrap themsevles in `{}` to begin with. + // + // TODO: This shouldn't be needed once we are more + // systematic about not emitting redundant parentheses. + // + switch(inst->op) + { + case kIROp_makeArray: + case kIROp_makeStruct: + emitIRInstExpr(ctx, inst, mode); + break; + + default: + emit("("); + emitIRInstExpr(ctx, inst, mode); + emit(")"); + break; + } return; } @@ -5884,10 +5911,17 @@ struct EmitVisitor auto returnInst = (IRReturnVal*) block->getLastInst(); SLANG_RELEASE_ASSERT(returnInst->op == kIROp_ReturnVal); - // Now we want to emit the expression form of - // the value being returned, and force any - // sub-expressions to be included. - emitIRInstExpr(ctx, returnInst->getVal(), IREmitMode::GlobalConstant); + // We will emit the value in the `GlobalConstant` mode, which + // more or less says to fold all instructions into their use + // sites, so that we end up with a single expression tree even + // in cases that would otherwise trip up our analysis. + // + // Note: We are emitting the value as an *operand* here instead + // of directly calling `emitIRInstExpr` because we need to handle + // cases where the value might *need* to emit as a named referenced + // (e.g., when it names another constant directly). + // + emitIROperand(ctx, returnInst->getVal(), IREmitMode::GlobalConstant); } void emitIRGlobalConstant( -- cgit v1.2.3