From 1b93de4f1df54ecad9a238506df668a56a3bfff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fl=C3=B6jt?= Date: Mon, 11 Nov 2019 11:30:22 +0100 Subject: [PATCH 1/2] Fix construction issue for 8 and 16 bit types. The problem is that constructing a float16_t, int8_t, uint8_t, int16_t, or uint16_t with a non 32-bit argument generates an aggregate with the wrong construction op. For int8_t and uint8_t, this resulted in a crash. --- glslang/MachineIndependent/ParseHelper.cpp | 50 +++++++++++++++++----- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp index 619bf81d..e31bff51 100644 --- a/glslang/MachineIndependent/ParseHelper.cpp +++ b/glslang/MachineIndependent/ParseHelper.cpp @@ -7017,8 +7017,14 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!intermediate.getArithemeticFloat16Enabled()) { TType tempType(EbtFloat, EvqTemporary, type.getVectorSize()); newNode = node; - if (tempType != newNode->getType()) - newNode = intermediate.setAggregateOperator(newNode, (TOperator)(EOpConstructVec2 + op - EOpConstructF16Vec2), tempType, node->getLoc()); + if (tempType != newNode->getType()) { + TOperator aggregateOp; + if (op == EOpConstructFloat16) + aggregateOp = EOpConstructFloat; + else + aggregateOp = (TOperator)(EOpConstructVec2 + op - EOpConstructF16Vec2); + newNode = intermediate.setAggregateOperator(newNode, aggregateOp, tempType, node->getLoc()); + } newNode = intermediate.addConversion(EbtFloat16, newNode); return newNode; } @@ -7034,8 +7040,14 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!intermediate.getArithemeticInt8Enabled()) { TType tempType(EbtInt, EvqTemporary, type.getVectorSize()); newNode = node; - if (tempType != newNode->getType()) - newNode = intermediate.setAggregateOperator(newNode, (TOperator)(EOpConstructIVec2 + op - EOpConstructI8Vec2), tempType, node->getLoc()); + if (tempType != newNode->getType()) { + TOperator aggregateOp; + if (op == EOpConstructInt8) + aggregateOp = EOpConstructInt; + else + aggregateOp = (TOperator)(EOpConstructIVec2 + op - EOpConstructI8Vec2); + newNode = intermediate.setAggregateOperator(newNode, aggregateOp, tempType, node->getLoc()); + } newNode = intermediate.addConversion(EbtInt8, newNode); return newNode; } @@ -7051,8 +7063,14 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!intermediate.getArithemeticInt8Enabled()) { TType tempType(EbtUint, EvqTemporary, type.getVectorSize()); newNode = node; - if (tempType != newNode->getType()) - newNode = intermediate.setAggregateOperator(newNode, (TOperator)(EOpConstructUVec2 + op - EOpConstructU8Vec2), tempType, node->getLoc()); + if (tempType != newNode->getType()) { + TOperator aggregateOp; + if (op == EOpConstructUint8) + aggregateOp = EOpConstructUint; + else + aggregateOp = (TOperator)(EOpConstructUVec2 + op - EOpConstructU8Vec2); + newNode = intermediate.setAggregateOperator(newNode, aggregateOp, tempType, node->getLoc()); + } newNode = intermediate.addConversion(EbtUint8, newNode); return newNode; } @@ -7068,8 +7086,14 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!intermediate.getArithemeticInt16Enabled()) { TType tempType(EbtInt, EvqTemporary, type.getVectorSize()); newNode = node; - if (tempType != newNode->getType()) - newNode = intermediate.setAggregateOperator(newNode, (TOperator)(EOpConstructIVec2 + op - EOpConstructI16Vec2), tempType, node->getLoc()); + if (tempType != newNode->getType()) { + TOperator aggregateOp; + if (op == EOpConstructInt16) + aggregateOp = EOpConstructInt; + else + aggregateOp = (TOperator)(EOpConstructIVec2 + op - EOpConstructI16Vec2); + newNode = intermediate.setAggregateOperator(newNode, aggregateOp, tempType, node->getLoc()); + } newNode = intermediate.addConversion(EbtInt16, newNode); return newNode; } @@ -7085,8 +7109,14 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!intermediate.getArithemeticInt16Enabled()) { TType tempType(EbtUint, EvqTemporary, type.getVectorSize()); newNode = node; - if (tempType != newNode->getType()) - newNode = intermediate.setAggregateOperator(newNode, (TOperator)(EOpConstructUVec2 + op - EOpConstructU16Vec2), tempType, node->getLoc()); + if (tempType != newNode->getType()) { + TOperator aggregateOp; + if (op == EOpConstructUint16) + aggregateOp = EOpConstructUint; + else + aggregateOp = (TOperator)(EOpConstructUVec2 + op - EOpConstructU16Vec2); + newNode = intermediate.setAggregateOperator(newNode, aggregateOp, tempType, node->getLoc()); + } newNode = intermediate.addConversion(EbtUint16, newNode); return newNode; } From cb563e68d2c67e1e0f09bb7760bd916a82698350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fl=C3=B6jt?= Date: Thu, 14 Nov 2019 14:41:52 +0100 Subject: [PATCH 2/2] Add a test for 8- and 16-bit construction. --- .../spv.8bit-16bit-construction.frag.out | 82 +++++++++++++++++++ Test/spv.8bit-16bit-construction.frag | 22 +++++ glslang/MachineIndependent/ParseHelper.cpp | 2 +- gtests/Spv.FromFile.cpp | 2 + 4 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 Test/baseResults/spv.8bit-16bit-construction.frag.out create mode 100644 Test/spv.8bit-16bit-construction.frag diff --git a/Test/baseResults/spv.8bit-16bit-construction.frag.out b/Test/baseResults/spv.8bit-16bit-construction.frag.out new file mode 100644 index 00000000..6cb9dc16 --- /dev/null +++ b/Test/baseResults/spv.8bit-16bit-construction.frag.out @@ -0,0 +1,82 @@ +spv.8bit-16bit-construction.frag +Validation failed +// Module Version 10000 +// Generated by (magic number): 80008 +// Id's are bound by 43 + + Capability Shader + Capability StorageUniformBufferBlock16 + Capability UniformAndStorageBuffer8BitAccess + Extension "SPV_KHR_16bit_storage" + Extension "SPV_KHR_8bit_storage" + 1: ExtInstImport "GLSL.std.450" + MemoryModel Logical GLSL450 + EntryPoint Fragment 4 "main" + ExecutionMode 4 OriginUpperLeft + Source GLSL 450 + SourceExtension "GL_EXT_shader_16bit_storage" + SourceExtension "GL_EXT_shader_8bit_storage" + Name 4 "main" + Name 11 "B" + MemberName 11(B) 0 "i8_from_i16" + MemberName 11(B) 1 "i16_from_i8" + MemberName 11(B) 2 "u8_from_u16" + MemberName 11(B) 3 "u16_from_u8" + MemberName 11(B) 4 "f16_from_i8" + Name 13 "" + MemberDecorate 11(B) 0 Offset 0 + MemberDecorate 11(B) 1 Offset 2 + MemberDecorate 11(B) 2 Offset 4 + MemberDecorate 11(B) 3 Offset 6 + MemberDecorate 11(B) 4 Offset 8 + Decorate 11(B) BufferBlock + Decorate 13 DescriptorSet 0 + Decorate 13 Binding 0 + 2: TypeVoid + 3: TypeFunction 2 + 6: TypeInt 8 1 + 7: TypeInt 16 1 + 8: TypeInt 8 0 + 9: TypeInt 16 0 + 10: TypeFloat 16 + 11(B): TypeStruct 6(int8_t) 7(int16_t) 8(int8_t) 9(int16_t) 10(float16_t) + 12: TypePointer Uniform 11(B) + 13: 12(ptr) Variable Uniform + 14: TypeInt 32 1 + 15: 14(int) Constant 0 + 16: 14(int) Constant 1 + 19: TypePointer Uniform 6(int8_t) + 23: TypePointer Uniform 7(int16_t) + 25: 14(int) Constant 2 + 26: TypeInt 32 0 + 27: 26(int) Constant 1 + 30: TypePointer Uniform 8(int8_t) + 32: 14(int) Constant 3 + 35: TypePointer Uniform 9(int16_t) + 37: 14(int) Constant 4 + 39: TypeFloat 32 + 41: TypePointer Uniform 10(float16_t) + 4(main): 2 Function None 3 + 5: Label + 17: 7(int16_t) SConvert 16 + 18: 6(int8_t) SConvert 17 + 20: 19(ptr) AccessChain 13 15 + Store 20 18 + 21: 6(int8_t) SConvert 16 + 22: 7(int16_t) SConvert 21 + 24: 23(ptr) AccessChain 13 16 + Store 24 22 + 28: 9(int16_t) UConvert 27 + 29: 8(int8_t) UConvert 28 + 31: 30(ptr) AccessChain 13 25 + Store 31 29 + 33: 8(int8_t) UConvert 27 + 34: 9(int16_t) UConvert 33 + 36: 35(ptr) AccessChain 13 32 + Store 36 34 + 38: 6(int8_t) SConvert 16 + 40:10(float16_t) FConvert 38 + 42: 41(ptr) AccessChain 13 37 + Store 42 40 + Return + FunctionEnd diff --git a/Test/spv.8bit-16bit-construction.frag b/Test/spv.8bit-16bit-construction.frag new file mode 100644 index 00000000..573fa548 --- /dev/null +++ b/Test/spv.8bit-16bit-construction.frag @@ -0,0 +1,22 @@ +#version 450 core + +#extension GL_EXT_shader_8bit_storage : enable +#extension GL_EXT_shader_16bit_storage : enable + +buffer B +{ + int8_t i8_from_i16; + int16_t i16_from_i8; + uint8_t u8_from_u16; + uint16_t u16_from_u8; + float16_t f16_from_i8; +}; + +void main() +{ + i8_from_i16 = int8_t(int16_t(1)); + i16_from_i8 = int16_t(int8_t(1)); + u8_from_u16 = uint8_t(uint16_t(1)); + u16_from_u8 = uint16_t(uint8_t(1)); + f16_from_i8 = float16_t(int8_t(1)); +} diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp index e31bff51..c5831302 100644 --- a/glslang/MachineIndependent/ParseHelper.cpp +++ b/glslang/MachineIndependent/ParseHelper.cpp @@ -2,7 +2,7 @@ // Copyright (C) 2002-2005 3Dlabs Inc. Ltd. // Copyright (C) 2012-2015 LunarG, Inc. // Copyright (C) 2015-2018 Google, Inc. -// Copyright (C) 2017 ARM Limited. +// Copyright (C) 2017, 2019 ARM Limited. // // All rights reserved. // diff --git a/gtests/Spv.FromFile.cpp b/gtests/Spv.FromFile.cpp index ce5960c8..0e46e790 100644 --- a/gtests/Spv.FromFile.cpp +++ b/gtests/Spv.FromFile.cpp @@ -1,5 +1,6 @@ // // Copyright (C) 2016 Google, Inc. +// Copyright (C) 2019 ARM Limited. // // All rights reserved. // @@ -263,6 +264,7 @@ INSTANTIATE_TEST_CASE_P( "spv.8bitstorage_Error-uint.frag", "spv.8bitstorage-ubo.vert", "spv.8bitstorage-ssbo.vert", + "spv.8bit-16bit-construction.frag", "spv.accessChain.frag", "spv.aggOps.frag", "spv.always-discard.frag",