From 0de16da2c065501862c24b881d259a2479bc3f50 Mon Sep 17 00:00:00 2001 From: steve-lunarg Date: Sat, 8 Oct 2016 10:54:52 -0600 Subject: [PATCH] HLSL: phase 2c: use lValueErrorCheck in HLSL FE This commit splits lValueErrorCheck into machine dependent and independent parts. The GLSL form in TParseContext inherits from and invokes the machine dependent part in TParseContextBase. The base form checks language independent things. This split does not change the set of errors tested for: the test results are identical. The new base class interface is now used from the HLSL FE to test lvalues. There was one test diff due to this, where the test was writing to a uniform. It still does the same indirections, but does not attempt a uniform write. --- Test/baseResults/hlsl.struct.frag.out | 120 +++++++++--------- Test/hlsl.struct.frag | 4 +- .../MachineIndependent/ParseContextBase.cpp | 108 ++++++++++++++++ glslang/MachineIndependent/ParseHelper.cpp | 71 +++-------- glslang/MachineIndependent/ParseHelper.h | 7 +- hlsl/hlslParseHelper.cpp | 41 ++++-- hlsl/hlslParseHelper.h | 1 + 7 files changed, 222 insertions(+), 130 deletions(-) diff --git a/Test/baseResults/hlsl.struct.frag.out b/Test/baseResults/hlsl.struct.frag.out index 04117d73..7605543f 100755 --- a/Test/baseResults/hlsl.struct.frag.out +++ b/Test/baseResults/hlsl.struct.frag.out @@ -14,15 +14,14 @@ gl_FragCoord origin is upper left 0:39 Compare Equal (temp bool) 0:39 's3' (temp structure{temp 3-component vector of bool b3}) 0:39 's3' (temp structure{temp 3-component vector of bool b3}) -0:40 move second child to first child (temp 4-component vector of float) -0:40 i: direct index for structure (temp 4-component vector of float) -0:40 s2: direct index for structure (layout(offset=48 ) uniform structure{temp 4-component vector of float i}) -0:40 'anon@0' (layout(row_major std140 ) uniform block{layout(offset=0 ) uniform structure{temp bool b, temp bool c, temp 4-component vector of float a, temp 4-component vector of float d} s1, layout(offset=48 ) uniform structure{temp 4-component vector of float i} s2, layout(binding=5 offset=1620 ) uniform float ff5, layout(binding=8 offset=1636 ) uniform float ff6}) -0:40 Constant: -0:40 1 (const uint) +0:40 i: direct index for structure (temp 4-component vector of float) +0:40 s2: direct index for structure (layout(offset=48 ) uniform structure{temp 4-component vector of float i}) +0:40 'anon@0' (layout(row_major std140 ) uniform block{layout(offset=0 ) uniform structure{temp bool b, temp bool c, temp 4-component vector of float a, temp 4-component vector of float d} s1, layout(offset=48 ) uniform structure{temp 4-component vector of float i} s2, layout(binding=5 offset=1620 ) uniform float ff5, layout(binding=8 offset=1636 ) uniform float ff6}) 0:40 Constant: -0:40 0 (const int) -0:? 'ff4' (layout(location=7 binding=0 offset=4 ) in 4-component vector of float) +0:40 1 (const uint) +0:40 Constant: +0:40 0 (const int) +0:? 'ff4' (layout(location=7 binding=0 offset=4 ) in 4-component vector of float) 0:42 Sequence 0:42 move second child to first child (temp 4-component vector of float) 0:? '@entryPointOutput' (layout(location=0 ) out 4-component vector of float) @@ -56,15 +55,14 @@ gl_FragCoord origin is upper left 0:39 Compare Equal (temp bool) 0:39 's3' (temp structure{temp 3-component vector of bool b3}) 0:39 's3' (temp structure{temp 3-component vector of bool b3}) -0:40 move second child to first child (temp 4-component vector of float) -0:40 i: direct index for structure (temp 4-component vector of float) -0:40 s2: direct index for structure (layout(offset=48 ) uniform structure{temp 4-component vector of float i}) -0:40 'anon@0' (layout(row_major std140 ) uniform block{layout(offset=0 ) uniform structure{temp bool b, temp bool c, temp 4-component vector of float a, temp 4-component vector of float d} s1, layout(offset=48 ) uniform structure{temp 4-component vector of float i} s2, layout(binding=5 offset=1620 ) uniform float ff5, layout(binding=8 offset=1636 ) uniform float ff6}) -0:40 Constant: -0:40 1 (const uint) +0:40 i: direct index for structure (temp 4-component vector of float) +0:40 s2: direct index for structure (layout(offset=48 ) uniform structure{temp 4-component vector of float i}) +0:40 'anon@0' (layout(row_major std140 ) uniform block{layout(offset=0 ) uniform structure{temp bool b, temp bool c, temp 4-component vector of float a, temp 4-component vector of float d} s1, layout(offset=48 ) uniform structure{temp 4-component vector of float i} s2, layout(binding=5 offset=1620 ) uniform float ff5, layout(binding=8 offset=1636 ) uniform float ff6}) 0:40 Constant: -0:40 0 (const int) -0:? 'ff4' (layout(location=7 binding=0 offset=4 ) in 4-component vector of float) +0:40 1 (const uint) +0:40 Constant: +0:40 0 (const int) +0:? 'ff4' (layout(location=7 binding=0 offset=4 ) in 4-component vector of float) 0:42 Sequence 0:42 move second child to first child (temp 4-component vector of float) 0:? '@entryPointOutput' (layout(location=0 ) out 4-component vector of float) @@ -85,12 +83,12 @@ gl_FragCoord origin is upper left // Module Version 10000 // Generated by (magic number): 80001 -// Id's are bound by 49 +// Id's are bound by 46 Capability Shader 1: ExtInstImport "GLSL.std.450" MemoryModel Logical GLSL450 - EntryPoint Fragment 4 "PixelShaderFunction" 29 34 35 38 40 42 45 46 47 48 + EntryPoint Fragment 4 "PixelShaderFunction" 29 31 32 35 37 39 42 43 44 45 ExecutionMode 4 OriginUpperLeft Name 4 "PixelShaderFunction" Name 8 "FS" @@ -110,15 +108,15 @@ gl_FragCoord origin is upper left MemberName 22($Global) 3 "ff6" Name 24 "" Name 29 "ff4" - Name 34 "@entryPointOutput" - Name 35 "input" - Name 38 "a" - Name 40 "b" - Name 42 "c" - Name 45 "d" - Name 46 "ff1" - Name 47 "ff2" - Name 48 "ff3" + Name 31 "@entryPointOutput" + Name 32 "input" + Name 35 "a" + Name 37 "b" + Name 39 "c" + Name 42 "d" + Name 43 "ff1" + Name 44 "ff2" + Name 45 "ff3" MemberDecorate 20(myS) 0 Offset 0 MemberDecorate 20(myS) 1 Offset 4 MemberDecorate 20(myS) 2 Offset 16 @@ -133,22 +131,22 @@ gl_FragCoord origin is upper left Decorate 29(ff4) Offset 4 Decorate 29(ff4) Location 7 Decorate 29(ff4) Binding 0 - Decorate 34(@entryPointOutput) Location 0 - Decorate 35(input) Location 0 - Decorate 38(a) Location 1 - Decorate 40(b) Flat - Decorate 40(b) Location 2 - Decorate 42(c) NoPerspective - Decorate 42(c) Centroid - Decorate 42(c) Location 3 - Decorate 45(d) Centroid - Decorate 45(d) Location 4 - Decorate 46(ff1) BuiltIn FrontFacing - Decorate 47(ff2) Offset 4 - Decorate 47(ff2) Location 5 - Decorate 48(ff3) Offset 4 - Decorate 48(ff3) Location 6 - Decorate 48(ff3) Binding 0 + Decorate 31(@entryPointOutput) Location 0 + Decorate 32(input) Location 0 + Decorate 35(a) Location 1 + Decorate 37(b) Flat + Decorate 37(b) Location 2 + Decorate 39(c) NoPerspective + Decorate 39(c) Centroid + Decorate 39(c) Location 3 + Decorate 42(d) Centroid + Decorate 42(d) Location 4 + Decorate 43(ff1) BuiltIn FrontFacing + Decorate 44(ff2) Offset 4 + Decorate 44(ff2) Location 5 + Decorate 45(ff3) Offset 4 + Decorate 45(ff3) Location 6 + Decorate 45(ff3) Binding 0 2: TypeVoid 3: TypeFunction 2 6: TypeBool @@ -168,21 +166,20 @@ gl_FragCoord origin is upper left 27: 25(int) Constant 0 28: TypePointer Input 19(fvec4) 29(ff4): 28(ptr) Variable Input - 31: TypePointer Uniform 19(fvec4) - 33: TypePointer Output 19(fvec4) -34(@entryPointOutput): 33(ptr) Variable Output - 35(input): 28(ptr) Variable Input - 38(a): 28(ptr) Variable Input - 39: TypePointer Input 6(bool) - 40(b): 39(ptr) Variable Input - 41: TypePointer Input 18(float) - 42(c): 41(ptr) Variable Input - 43: TypeVector 18(float) 2 - 44: TypePointer Input 43(fvec2) - 45(d): 44(ptr) Variable Input - 46(ff1): 39(ptr) Variable Input - 47(ff2): 39(ptr) Variable Input - 48(ff3): 39(ptr) Variable Input + 30: TypePointer Output 19(fvec4) +31(@entryPointOutput): 30(ptr) Variable Output + 32(input): 28(ptr) Variable Input + 35(a): 28(ptr) Variable Input + 36: TypePointer Input 6(bool) + 37(b): 36(ptr) Variable Input + 38: TypePointer Input 18(float) + 39(c): 38(ptr) Variable Input + 40: TypeVector 18(float) 2 + 41: TypePointer Input 40(fvec2) + 42(d): 41(ptr) Variable Input + 43(ff1): 36(ptr) Variable Input + 44(ff2): 36(ptr) Variable Input + 45(ff3): 36(ptr) Variable Input 4(PixelShaderFunction): 2 Function None 3 5: Label 10(s3): 9(ptr) Variable Function @@ -192,10 +189,7 @@ gl_FragCoord origin is upper left 14: 7(bvec3) CompositeExtract 12 0 15: 7(bvec3) LogicalEqual 13 14 16: 6(bool) All 15 - 30: 19(fvec4) Load 29(ff4) - 32: 31(ptr) AccessChain 24 26 27 - Store 32 30 - 36: 19(fvec4) Load 35(input) - Store 34(@entryPointOutput) 36 + 33: 19(fvec4) Load 32(input) + Store 31(@entryPointOutput) 33 Return FunctionEnd diff --git a/Test/hlsl.struct.frag b/Test/hlsl.struct.frag index 2c511a6e..456c9ef7 100644 --- a/Test/hlsl.struct.frag +++ b/Test/hlsl.struct.frag @@ -37,7 +37,7 @@ float4 PixelShaderFunction(float4 input, IN_S s) : COLOR0 } s3; s3 == s3; - s2.i = s.ff4; + s2.i; s.ff4; // no assignments to uniforms, but preserve indirections. return input; -} \ No newline at end of file +} diff --git a/glslang/MachineIndependent/ParseContextBase.cpp b/glslang/MachineIndependent/ParseContextBase.cpp index fe2b8e6b..45bb1689 100644 --- a/glslang/MachineIndependent/ParseContextBase.cpp +++ b/glslang/MachineIndependent/ParseContextBase.cpp @@ -113,6 +113,114 @@ void C_DECL TParseContextBase::ppWarn(const TSourceLoc& loc, const char* szReaso va_end(args); } +// +// Both test and if necessary, spit out an error, to see if the node is really +// an l-value that can be operated on this way. +// +// Returns true if there was an error. +// +bool TParseContextBase::lValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) +{ + TIntermBinary* binaryNode = node->getAsBinaryNode(); + + if (binaryNode) { + switch(binaryNode->getOp()) { + case EOpIndexDirect: + case EOpIndexIndirect: // fall through + case EOpIndexDirectStruct: // fall through + case EOpVectorSwizzle: + return lValueErrorCheck(loc, op, binaryNode->getLeft()); + default: + break; + } + error(loc, " l-value required", op, "", ""); + + return true; + } + + const char* symbol = nullptr; + TIntermSymbol* symNode = node->getAsSymbolNode(); + if (symNode != nullptr) + symbol = symNode->getName().c_str(); + + const char* message = nullptr; + switch (node->getQualifier().storage) { + case EvqConst: message = "can't modify a const"; break; + case EvqConstReadOnly: message = "can't modify a const"; break; + case EvqUniform: message = "can't modify a uniform"; break; + case EvqBuffer: + if (node->getQualifier().readonly) + message = "can't modify a readonly buffer"; + break; + + default: + // + // Type that can't be written to? + // + switch (node->getBasicType()) { + case EbtSampler: + message = "can't modify a sampler"; + break; + case EbtAtomicUint: + message = "can't modify an atomic_uint"; + break; + case EbtVoid: + message = "can't modify void"; + break; + default: + break; + } + } + + if (message == nullptr && binaryNode == nullptr && symNode == nullptr) { + error(loc, " l-value required", op, "", ""); + + return true; + } + + // + // Everything else is okay, no error. + // + if (message == nullptr) + return false; + + // + // If we get here, we have an error and a message. + // + if (symNode) + error(loc, " l-value required", op, "\"%s\" (%s)", symbol, message); + else + error(loc, " l-value required", op, "(%s)", message); + + return true; +} + +// Test for and give an error if the node can't be read from. +void TParseContextBase::rValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) +{ + if (! node) + return; + + TIntermBinary* binaryNode = node->getAsBinaryNode(); + if (binaryNode) { + switch(binaryNode->getOp()) { + case EOpIndexDirect: + case EOpIndexIndirect: + case EOpIndexDirectStruct: + case EOpVectorSwizzle: + rValueErrorCheck(loc, op, binaryNode->getLeft()); + default: + break; + } + + return; + } + + TIntermSymbol* symNode = node->getAsSymbolNode(); + if (symNode && symNode->getQualifier().writeonly) + error(loc, "can't read from writeonly object: ", op, symNode->getName().c_str()); +} + // Make a shared symbol have a non-shared version that can be edited by the current // compile, such that editing its type will not change the shared version and will // effect all nodes sharing it. diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp index 949ba007..de1e8ca7 100644 --- a/glslang/MachineIndependent/ParseHelper.cpp +++ b/glslang/MachineIndependent/ParseHelper.cpp @@ -1902,14 +1902,14 @@ void TParseContext::variableCheck(TIntermTyped*& nodePtr) // Both test and if necessary, spit out an error, to see if the node is really // an l-value that can be operated on this way. // -// Returns true if the was an error. +// Returns true if there was an error. // bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) { TIntermBinary* binaryNode = node->getAsBinaryNode(); if (binaryNode) { - bool errorReturn; + bool errorReturn = false; switch(binaryNode->getOp()) { case EOpIndexDirect: @@ -1928,9 +1928,9 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt } } - // fall through + break; // left node is checked by base class case EOpIndexDirectStruct: - return lValueErrorCheck(loc, op, binaryNode->getLeft()); + break; // left node is checked by base class case EOpVectorSwizzle: errorReturn = lValueErrorCheck(loc, op, binaryNode->getLeft()); if (!errorReturn) { @@ -1955,12 +1955,18 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt default: break; } - error(loc, " l-value required", op, "", ""); - return true; + if (errorReturn) { + error(loc, " l-value required", op, "", ""); + return true; + } } + // Let the base class check errors + if (TParseContextBase::lValueErrorCheck(loc, op, node)) + return true; + // GLSL specific const char* symbol = nullptr; TIntermSymbol* symNode = node->getAsSymbolNode(); if (symNode != nullptr) @@ -1968,19 +1974,12 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt const char* message = nullptr; switch (node->getQualifier().storage) { - case EvqConst: message = "can't modify a const"; break; - case EvqConstReadOnly: message = "can't modify a const"; break; case EvqVaryingIn: message = "can't modify shader input"; break; case EvqInstanceId: message = "can't modify gl_InstanceID"; break; case EvqVertexId: message = "can't modify gl_VertexID"; break; case EvqFace: message = "can't modify gl_FrontFace"; break; case EvqFragCoord: message = "can't modify gl_FragCoord"; break; case EvqPointCoord: message = "can't modify gl_PointCoord"; break; - case EvqUniform: message = "can't modify a uniform"; break; - case EvqBuffer: - if (node->getQualifier().readonly) - message = "can't modify a readonly buffer"; - break; case EvqFragDepth: intermediate.setDepthReplacing(); // "In addition, it is an error to statically write to gl_FragDepth in the fragment shader." @@ -1989,22 +1988,7 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt break; default: - // - // Type that can't be written to? - // - switch (node->getBasicType()) { - case EbtSampler: - message = "can't modify a sampler"; - break; - case EbtAtomicUint: - message = "can't modify an atomic_uint"; - break; - case EbtVoid: - message = "can't modify void"; - break; - default: - break; - } + break; } if (message == nullptr && binaryNode == nullptr && symNode == nullptr) { @@ -2013,7 +1997,6 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt return true; } - // // Everything else is okay, no error. // @@ -2034,30 +2017,14 @@ bool TParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TInt // Test for and give an error if the node can't be read from. void TParseContext::rValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) { - if (! node) - return; + // Let the base class check errors + TParseContextBase::rValueErrorCheck(loc, op, node); - TIntermBinary* binaryNode = node->getAsBinaryNode(); - if (binaryNode) { - switch(binaryNode->getOp()) { - case EOpIndexDirect: - case EOpIndexIndirect: - case EOpIndexDirectStruct: - case EOpVectorSwizzle: - rValueErrorCheck(loc, op, binaryNode->getLeft()); - default: - break; - } - - return; - } - - TIntermSymbol* symNode = node->getAsSymbolNode(); - if (symNode && symNode->getQualifier().writeonly) - error(loc, "can't read from writeonly object: ", op, symNode->getName().c_str()); #ifdef AMD_EXTENSIONS - else if (symNode && symNode->getQualifier().explicitInterp) - error(loc, "can't read from explicitly-interpolated object: ", op, symNode->getName().c_str()); + TIntermSymbol* symNode = node->getAsSymbolNode(); + if (!(symNode && symNode->getQualifier().writeonly)) // base class checks + if (symNode && symNode->getQualifier().explicitInterp) + error(loc, "can't read from explicitly-interpolated object: ", op, symNode->getName().c_str()); #endif } diff --git a/glslang/MachineIndependent/ParseHelper.h b/glslang/MachineIndependent/ParseHelper.h index eab78359..0f60ff7f 100644 --- a/glslang/MachineIndependent/ParseHelper.h +++ b/glslang/MachineIndependent/ParseHelper.h @@ -143,6 +143,9 @@ public: virtual void growGlobalUniformBlock(TSourceLoc&, TType&, TString& memberName); virtual bool insertGlobalUniformBlock(); + virtual bool lValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*); + virtual void rValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*); + protected: TParseContextBase(TParseContextBase&); TParseContextBase& operator=(TParseContextBase&); @@ -278,8 +281,8 @@ public: void unaryOpError(const TSourceLoc&, const char* op, TString operand); void binaryOpError(const TSourceLoc&, const char* op, TString left, TString right); void variableCheck(TIntermTyped*& nodePtr); - bool lValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*); - void rValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*); + bool lValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*) override; + void rValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*) override; void constantValueCheck(TIntermTyped* node, const char* token); void integerCheck(const TIntermTyped* node, const char* token); void globalCheck(const TSourceLoc&, const char* token); diff --git a/hlsl/hlslParseHelper.cpp b/hlsl/hlslParseHelper.cpp index 5eccf61c..5c1fd059 100755 --- a/hlsl/hlslParseHelper.cpp +++ b/hlsl/hlslParseHelper.cpp @@ -136,12 +136,36 @@ bool HlslParseContext::shouldConvertLValue(const TIntermNode* node) const return false; const TIntermAggregate* lhsAsAggregate = node->getAsAggregate(); + if (lhsAsAggregate != nullptr && lhsAsAggregate->getOp() == EOpImageLoad) return true; return false; } +// +// Both test and if necessary, spit out an error, to see if the node is really +// an l-value that can be operated on this way. +// +// Returns true if there was an error. +// +bool HlslParseContext::lValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) +{ + if (shouldConvertLValue(node)) { + // if we're writing to a texture, it must be an RW form. + + TIntermAggregate* lhsAsAggregate = node->getAsAggregate(); + TIntermTyped* object = lhsAsAggregate->getSequence()[0]->getAsTyped(); + + if (!object->getType().getSampler().isImage()) { + error(loc, "operator[] on a non-RW texture must be an r-value", "", ""); + return true; + } + } + + // Let the base class check errors + return TParseContextBase::lValueErrorCheck(loc, op, node); +} // // This function handles l-value conversions and verifications. It uses, but is not synonymous @@ -161,13 +185,11 @@ TIntermTyped* HlslParseContext::handleLvalue(const TSourceLoc& loc, const char* nodeAsBinary ? nodeAsBinary->getLeft() : nullptr; - // Early bail out if there is no conversion to apply if (!shouldConvertLValue(lhs)) { - // TODO: >.. - // if (lhs != nullptr) - // if (lValueErrorCheck(loc, op, lhs)) - // return nullptr; + if (lhs != nullptr) + if (lValueErrorCheck(loc, op, lhs)) + return nullptr; return node; } @@ -356,8 +378,6 @@ TIntermTyped* HlslParseContext::handleLvalue(const TSourceLoc& loc, const char* addUnary(assignOp, rhsTmp2); // rhsTmp op makeStore(object, coordTmp, rhsTmp2); // OpImageStore(object, coordTmp, rhsTmp2) return finishSequence(rhsTmp1, objDerefType); // return rhsTmp from sequence - - break; } default: @@ -365,10 +385,9 @@ TIntermTyped* HlslParseContext::handleLvalue(const TSourceLoc& loc, const char* } } - // TODO: - // if (lhs) - // if (lValueErrorCheck(loc, op, lhs)) - // return nullptr; + if (lhs) + if (lValueErrorCheck(loc, op, lhs)) + return nullptr; return node; } diff --git a/hlsl/hlslParseHelper.h b/hlsl/hlslParseHelper.h index 8dcfe840..d8e75466 100755 --- a/hlsl/hlslParseHelper.h +++ b/hlsl/hlslParseHelper.h @@ -154,6 +154,7 @@ public: // Apply L-value conversions. E.g, turning a write to a RWTexture into an ImageStore. TIntermTyped* handleLvalue(const TSourceLoc&, const char* op, TIntermTyped* node); + bool lValueErrorCheck(const TSourceLoc&, const char* op, TIntermTyped*) override; protected: void inheritGlobalDefaults(TQualifier& dst) const;