From a4342f3f442178e79e79a8c7bbbfba0c8ca851a7 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Fri, 22 Jan 2016 14:27:00 -0500 Subject: [PATCH] Remove spvOpcodeIsObject(). Also - Add type_id to spv_id_info_t. - Use spv_id_info_t::type_id instead of words[1]. Triggered some asserts on tests, where the code incorrectly assumed words[1] had a type. Remove the asserts and handle gracefully. - Add tests for OpStore of a label, a void, and a function. --- source/opcode.cpp | 115 ------------------------------------------------- source/opcode.h | 4 -- source/validate.cpp | 2 +- source/validate.h | 2 + source/validate_id.cpp | 55 ++++++++++++----------- test/ValidateID.cpp | 47 ++++++++++++++++++++ 6 files changed, 77 insertions(+), 148 deletions(-) diff --git a/source/opcode.cpp b/source/opcode.cpp index 2fbe81e..fec64a1 100644 --- a/source/opcode.cpp +++ b/source/opcode.cpp @@ -489,121 +489,6 @@ int32_t spvOpcodeIsPointer(const SpvOp opcode) { } } -int32_t spvOpcodeIsObject(const SpvOp opcode) { - switch (opcode) { - case SpvOpConstantTrue: - case SpvOpConstantFalse: - case SpvOpConstant: - case SpvOpConstantComposite: - // TODO: case SpvOpConstantSampler: - case SpvOpConstantNull: - case SpvOpSpecConstantTrue: - case SpvOpSpecConstantFalse: - case SpvOpSpecConstant: - case SpvOpSpecConstantComposite: - // TODO: case SpvOpSpecConstantOp: - case SpvOpVariable: - case SpvOpAccessChain: - case SpvOpInBoundsAccessChain: - case SpvOpConvertFToU: - case SpvOpConvertFToS: - case SpvOpConvertSToF: - case SpvOpConvertUToF: - case SpvOpUConvert: - case SpvOpSConvert: - case SpvOpFConvert: - case SpvOpConvertPtrToU: - // TODO: case SpvOpConvertUToPtr: - case SpvOpPtrCastToGeneric: - // TODO: case SpvOpGenericCastToPtr: - case SpvOpBitcast: - // TODO: case SpvOpGenericCastToPtrExplicit: - case SpvOpSatConvertSToU: - case SpvOpSatConvertUToS: - case SpvOpVectorExtractDynamic: - case SpvOpCompositeConstruct: - case SpvOpCompositeExtract: - case SpvOpCopyObject: - case SpvOpTranspose: - case SpvOpSNegate: - case SpvOpFNegate: - case SpvOpNot: - case SpvOpIAdd: - case SpvOpFAdd: - case SpvOpISub: - case SpvOpFSub: - case SpvOpIMul: - case SpvOpFMul: - case SpvOpUDiv: - case SpvOpSDiv: - case SpvOpFDiv: - case SpvOpUMod: - case SpvOpSRem: - case SpvOpSMod: - case SpvOpVectorTimesScalar: - case SpvOpMatrixTimesScalar: - case SpvOpVectorTimesMatrix: - case SpvOpMatrixTimesVector: - case SpvOpMatrixTimesMatrix: - case SpvOpOuterProduct: - case SpvOpDot: - case SpvOpShiftRightLogical: - case SpvOpShiftRightArithmetic: - case SpvOpShiftLeftLogical: - case SpvOpBitwiseOr: - case SpvOpBitwiseXor: - case SpvOpBitwiseAnd: - case SpvOpAny: - case SpvOpAll: - case SpvOpIsNan: - case SpvOpIsInf: - case SpvOpIsFinite: - case SpvOpIsNormal: - case SpvOpSignBitSet: - case SpvOpLessOrGreater: - case SpvOpOrdered: - case SpvOpUnordered: - case SpvOpLogicalOr: - case SpvOpLogicalAnd: - case SpvOpSelect: - case SpvOpIEqual: - case SpvOpFOrdEqual: - case SpvOpFUnordEqual: - case SpvOpINotEqual: - case SpvOpFOrdNotEqual: - case SpvOpFUnordNotEqual: - case SpvOpULessThan: - case SpvOpSLessThan: - case SpvOpFOrdLessThan: - case SpvOpFUnordLessThan: - case SpvOpUGreaterThan: - case SpvOpSGreaterThan: - case SpvOpFOrdGreaterThan: - case SpvOpFUnordGreaterThan: - case SpvOpULessThanEqual: - case SpvOpSLessThanEqual: - case SpvOpFOrdLessThanEqual: - case SpvOpFUnordLessThanEqual: - case SpvOpUGreaterThanEqual: - case SpvOpSGreaterThanEqual: - case SpvOpFOrdGreaterThanEqual: - case SpvOpFUnordGreaterThanEqual: - case SpvOpDPdx: - case SpvOpDPdy: - case SpvOpFwidth: - case SpvOpDPdxFine: - case SpvOpDPdyFine: - case SpvOpFwidthFine: - case SpvOpDPdxCoarse: - case SpvOpDPdyCoarse: - case SpvOpFwidthCoarse: - case SpvOpReturnValue: - return true; - default: - return false; - } -} - int32_t spvOpcodeIsBasicTypeNullable(SpvOp opcode) { switch (opcode) { case SpvOpTypeBool: diff --git a/source/opcode.h b/source/opcode.h index 1088863..7ba2b96 100644 --- a/source/opcode.h +++ b/source/opcode.h @@ -92,10 +92,6 @@ int32_t spvOpcodeAreTypesEqual(const spv_instruction_t* type_inst0, // non-zero otherwise. int32_t spvOpcodeIsPointer(const SpvOp opcode); -// Determines if the given opcode results in an instantation of a non-void type. -// Returns zero if false, non-zero otherwise. -int32_t spvOpcodeIsObject(const SpvOp opcode); - // Determines if the scalar type opcode is nullable. Returns zero if false, // non-zero otherwise. int32_t spvOpcodeIsBasicTypeNullable(SpvOp opcode); diff --git a/source/validate.cpp b/source/validate.cpp index a09eb12..f2b5f26 100644 --- a/source/validate.cpp +++ b/source/validate.cpp @@ -274,7 +274,7 @@ void DebugInstructionPass(ValidationState_t& _, void ProcessIds(ValidationState_t& _, const spv_parsed_instruction_t& inst) { if (inst.result_id) { _.usedefs().AddDef( - {inst.result_id, inst.opcode, + {inst.result_id, inst.type_id, inst.opcode, std::vector(inst.words, inst.words + inst.num_words)}); } for (auto op = inst.operands; op != inst.operands + inst.num_operands; ++op) { diff --git a/source/validate.h b/source/validate.h index d0c053e..b94fed2 100644 --- a/source/validate.h +++ b/source/validate.h @@ -47,6 +47,8 @@ typedef struct spv_id_info_t { // Id value. uint32_t id; + // Type id, or 0 if no type. + uint32_t type_id; // Opcode of the instruction defining the id. SpvOp opcode; // Binary words of the instruction defining the id. diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 9c42586..47eba66 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -197,7 +197,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "'s function parameter count is not zero."; return false; } - auto returnType = usedefs_.FindDef(entryPoint.second.words[1]); + auto returnType = usedefs_.FindDef(entryPoint.second.type_id); if (!returnType.first || SpvOpTypeVoid != returnType.second.opcode) { DIAG(entryPointIndex) << "OpEntryPoint Entry Point '" << inst->words[entryPointIndex] @@ -454,7 +454,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, return false; } auto constituentResultType = - usedefs_.FindDef(constituent.second.words[1]); + usedefs_.FindDef(constituent.second.type_id); if (!constituentResultType.first || componentType.second.opcode != constituentResultType.second.opcode) { @@ -494,7 +494,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a constant composite."; return false; } - auto vector = usedefs_.FindDef(constituent.second.words[1]); + auto vector = usedefs_.FindDef(constituent.second.type_id); assert(vector.first); spvCheck(columnType.second.opcode != vector.second.opcode, DIAG(constituentIndex) @@ -544,7 +544,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a constant."; return false; } - auto constituentType = usedefs_.FindDef(constituent.second.words[1]); + auto constituentType = usedefs_.FindDef(constituent.second.type_id); assert(constituentType.first); spvCheck(elementType.second.id != constituentType.second.id, DIAG(constituentIndex) @@ -575,7 +575,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a constant."; return false; } - auto constituentType = usedefs_.FindDef(constituent.second.words[1]); + auto constituentType = usedefs_.FindDef(constituent.second.type_id); assert(constituentType.first); auto memberType = @@ -751,7 +751,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a pointer."; return false; } - auto pointerType = usedefs_.FindDef(pointer.second.words[1]); + auto pointerType = usedefs_.FindDef(pointer.second.type_id); if (!pointerType.first || pointerType.second.opcode != SpvOpTypePointer) { DIAG(pointerIndex) << "OpLoad type for pointer '" << inst->words[pointerIndex] @@ -779,7 +779,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a pointer."; return false; } - auto pointerType = usedefs_.FindDef(pointer.second.words[1]); + auto pointerType = usedefs_.FindDef(pointer.second.type_id); assert(pointerType.first); auto type = usedefs_.FindDef(pointerType.second.words[3]); assert(type.first); @@ -791,12 +791,12 @@ bool idUsage::isValid(const spv_instruction_t* inst, auto objectIndex = 2; auto object = usedefs_.FindDef(inst->words[objectIndex]); - if (!object.first || !spvOpcodeIsObject(object.second.opcode)) { + if (!object.first || !object.second.type_id) { DIAG(objectIndex) << "OpStore Object '" << inst->words[objectIndex] - << "' in not an object."; + << "' is not an object."; return false; } - auto objectType = usedefs_.FindDef(object.second.words[1]); + auto objectType = usedefs_.FindDef(object.second.type_id); assert(objectType.first); spvCheck(SpvOpTypeVoid == objectType.second.opcode, DIAG(objectIndex) << "OpStore Object '" @@ -821,11 +821,11 @@ bool idUsage::isValid(const spv_instruction_t* inst, auto sourceIndex = 2; auto source = usedefs_.FindDef(inst->words[sourceIndex]); if (!source.first) return false; - auto targetPointerType = usedefs_.FindDef(target.second.words[1]); + auto targetPointerType = usedefs_.FindDef(target.second.type_id); assert(targetPointerType.first); auto targetType = usedefs_.FindDef(targetPointerType.second.words[3]); assert(targetType.first); - auto sourcePointerType = usedefs_.FindDef(source.second.words[1]); + auto sourcePointerType = usedefs_.FindDef(source.second.type_id); assert(sourcePointerType.first); auto sourceType = usedefs_.FindDef(sourcePointerType.second.words[3]); assert(sourceType.first); @@ -850,16 +850,16 @@ bool idUsage::isValid(const spv_instruction_t* inst, auto sizeIndex = 3; auto size = usedefs_.FindDef(inst->words[sizeIndex]); if (!size.first) return false; - auto targetPointerType = usedefs_.FindDef(target.second.words[1]); - assert(targetPointerType.first); - spvCheck(SpvOpTypePointer != targetPointerType.second.opcode, + auto targetPointerType = usedefs_.FindDef(target.second.type_id); + spvCheck(!targetPointerType.first || + SpvOpTypePointer != targetPointerType.second.opcode, DIAG(targetIndex) << "OpCopyMemorySized Target '" << inst->words[targetIndex] << "' is not a pointer."; return false); - auto sourcePointerType = usedefs_.FindDef(source.second.words[1]); - assert(sourcePointerType.first); - spvCheck(SpvOpTypePointer != sourcePointerType.second.opcode, + auto sourcePointerType = usedefs_.FindDef(source.second.type_id); + spvCheck(!sourcePointerType.first || + SpvOpTypePointer != sourcePointerType.second.opcode, DIAG(sourceIndex) << "OpCopyMemorySized Source '" << inst->words[sourceIndex] << "' is not a pointer."; @@ -870,7 +870,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, // clarification case SpvOpConstant: case SpvOpSpecConstant: { - auto sizeType = usedefs_.FindDef(size.second.words[1]); + auto sizeType = usedefs_.FindDef(size.second.type_id); assert(sizeType.first); spvCheck(SpvOpTypeInt != sizeType.second.opcode, DIAG(sizeIndex) << "OpCopyMemorySized Size '" @@ -879,11 +879,10 @@ bool idUsage::isValid(const spv_instruction_t* inst, return false); } break; case SpvOpVariable: { - auto pointerType = usedefs_.FindDef(size.second.words[1]); + auto pointerType = usedefs_.FindDef(size.second.type_id); assert(pointerType.first); - auto sizeType = usedefs_.FindDef(pointerType.second.words[1]); - assert(sizeType.first); - spvCheck(SpvOpTypeInt != sizeType.second.opcode, + auto sizeType = usedefs_.FindDef(pointerType.second.type_id); + spvCheck(!sizeType.first || SpvOpTypeInt != sizeType.second.opcode, DIAG(sizeIndex) << "OpCopyMemorySized Size '" << inst->words[sizeIndex] << "'s variable type is not an integer type."; @@ -1003,7 +1002,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << inst->words[functionIndex] << "' is not a function."; return false; } - auto returnType = usedefs_.FindDef(function.second.words[1]); + auto returnType = usedefs_.FindDef(function.second.type_id); assert(returnType.first); spvCheck(returnType.second.id != resultType.second.id, DIAG(resultTypeIndex) << "OpFunctionCall Result Type '" @@ -1025,7 +1024,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, argumentIndex < inst->words.size(); argumentIndex++, paramIndex++) { auto argument = usedefs_.FindDef(inst->words[argumentIndex]); if (!argument.first) return false; - auto argumentType = usedefs_.FindDef(argument.second.words[1]); + auto argumentType = usedefs_.FindDef(argument.second.type_id); assert(argumentType.first); auto parameterType = usedefs_.FindDef(functionType.second.words[paramIndex]); @@ -1677,7 +1676,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' does not represent a value."; return false; } - auto valueType = usedefs_.FindDef(value.second.words[1]); + auto valueType = usedefs_.FindDef(value.second.type_id); assert(valueType.first); // NOTE: Find OpFunction const spv_instruction_t* function = inst - 1; @@ -2098,8 +2097,8 @@ bool idUsage::isValid(const spv_instruction_t* inst) { #define CASE(OpCode) \ case Spv##OpCode: \ return isValid(inst, opcodeEntry); -#define TODO(OpCode) \ - case Spv##OpCode: \ +#define TODO(OpCode) \ + case Spv##OpCode: \ return true; switch (inst->opcode) { TODO(OpUndef) diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index f653eb4..e7dacff 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -821,6 +821,53 @@ TEST_F(ValidateID, OpStoreTypeBad) { CHECK(spirv, SPV_ERROR_INVALID_ID); } +TEST_F(ValidateID, OpStoreVoid) { + const char* spirv = R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 1 +%3 = OpTypePointer UniformConstant %2 +%4 = OpTypeFunction %1 +%6 = OpVariable %3 UniformConstant +%7 = OpFunction %1 None %4 +%8 = OpLabel +%9 = OpFunctionCall %1 %7 + OpStore %6 %9 + OpReturn + OpFunctionEnd)"; + CHECK(spirv, SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpStoreLabel) { + const char* spirv = R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 1 +%3 = OpTypePointer UniformConstant %2 +%4 = OpTypeFunction %1 +%6 = OpVariable %3 UniformConstant +%7 = OpFunction %1 None %4 +%8 = OpLabel + OpStore %6 %8 + OpReturn + OpFunctionEnd)"; + CHECK(spirv, SPV_ERROR_INVALID_ID); +} + +// TODO: enable when this bug is fixed: https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15404 +TEST_F(ValidateID, DISABLED_OpStoreFunction) { + const char* spirv = R"( +%2 = OpTypeInt 32 1 +%3 = OpTypePointer UniformConstant %2 +%4 = OpTypeFunction %2 +%5 = OpConstant %2 123 +%6 = OpVariable %3 UniformConstant +%7 = OpFunction %2 None %4 +%8 = OpLabel + OpStore %6 %7 + OpReturnValue %5 + OpFunctionEnd)"; + CHECK(spirv, SPV_ERROR_INVALID_ID); +} + TEST_F(ValidateID, OpCopyMemoryGood) { const char* spirv = R"( %1 = OpTypeVoid -- 2.7.4