SpirvShader: Rework debug assertions
Use the new UNSUPPORTED() macro where possible.
Switch to using UNREACHABLE() over UNIMPLEMENTED() for switch cases that do not exist according to the spec, or for cases that should be handled by validation in an earlier stage.
Bug: b/131243109
Change-Id: Id9970b64cf248468c508e65366bc576cb03e8e58
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/29929
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
Presubmit-Ready: Ben Clayton <bclayton@google.com>
Tested-by: Ben Clayton <bclayton@google.com>
diff --git a/src/Pipeline/SpirvShader.cpp b/src/Pipeline/SpirvShader.cpp
index b3c7abe..073f0da 100644
--- a/src/Pipeline/SpirvShader.cpp
+++ b/src/Pipeline/SpirvShader.cpp
@@ -561,13 +561,19 @@
break; // Correctly handled.
case spv::StorageClassWorkgroup:
- case spv::StorageClassCrossWorkgroup:
- case spv::StorageClassGeneric:
case spv::StorageClassAtomicCounter:
case spv::StorageClassImage:
UNIMPLEMENTED("StorageClass %d not yet implemented", (int)storageClass);
break;
+ case spv::StorageClassCrossWorkgroup:
+ UNSUPPORTED("SPIR-V OpenCL Execution Model (StorageClassCrossWorkgroup)");
+ break;
+
+ case spv::StorageClassGeneric:
+ UNSUPPORTED("SPIR-V GenericPointer Capability (StorageClassGeneric)");
+ break;
+
default:
UNREACHABLE("Unexpected StorageClass %d", storageClass); // See Appendix A of the Vulkan spec.
break;
@@ -660,13 +666,16 @@
// Due to preprocessing, the entrypoint and its function provide no value.
break;
case spv::OpExtInstImport:
+ {
// We will only support the GLSL 450 extended instruction set, so no point in tracking the ID we assign it.
// Valid shaders will not attempt to import any other instruction sets.
- if (0 != strcmp("GLSL.std.450", reinterpret_cast<char const *>(insn.wordPointer(2))))
+ auto ext = reinterpret_cast<char const *>(insn.wordPointer(2));
+ if (0 != strcmp("GLSL.std.450", ext))
{
- UNIMPLEMENTED("Only GLSL extended instruction set is supported");
+ UNSUPPORTED("SPIR-V Extension: %s", ext);
}
break;
+ }
case spv::OpName:
case spv::OpMemberName:
case spv::OpSource:
@@ -688,16 +697,19 @@
case spv::OpSpecConstantTrue:
// These should have all been removed by preprocessing passes. If we see them here,
// our assumptions are wrong and we will probably generate wrong code.
- UNIMPLEMENTED("%s should have already been lowered.", OpcodeName(opcode).c_str());
+ UNREACHABLE("%s should have already been lowered.", OpcodeName(opcode).c_str());
break;
case spv::OpFConvert:
- UNIMPLEMENTED("No valid uses for OpFConvert until we support multiple bit widths enabled by features such as Float16/Float64 etc.");
+ UNSUPPORTED("SPIR-V Float16 or Float64 Capability (OpFConvert)");
break;
case spv::OpSConvert:
+ UNSUPPORTED("SPIR-V Int16 or Int64 Capability (OpSConvert)");
+ break;
+
case spv::OpUConvert:
- UNIMPLEMENTED("No valid uses for Op*Convert until we support multiple bit widths enabled by features such as Int16/Int64 etc.");
+ UNSUPPORTED("SPIR-V Int16 or Int64 Capability (OpUConvert)");
break;
case spv::OpLoad:
@@ -863,11 +875,11 @@
case spv::OpExtension:
{
- auto p = reinterpret_cast<char const *>(insn.wordPointer(1));
+ auto ext = reinterpret_cast<char const *>(insn.wordPointer(1));
// Part of core SPIR-V 1.3. Vulkan 1.1 implementations must also accept the pre-1.3
// extension per Appendix A, `Vulkan Environment for SPIR-V`.
- if (!strcmp(p, "SPV_KHR_storage_buffer_storage_class")) break;
- UNIMPLEMENTED("Unknown extension %s", p);
+ if (!strcmp(ext, "SPV_KHR_storage_buffer_storage_class")) break;
+ UNSUPPORTED("SPIR-V Extension: %s", ext);
break;
}
@@ -1065,7 +1077,7 @@
// This is always the case for a Vulkan shader. Do nothing.
break;
default:
- UNIMPLEMENTED("No other execution modes are permitted");
+ UNREACHABLE("Execution mode: %d", int(mode));
}
}
@@ -1122,8 +1134,7 @@
return 1;
default:
- // Some other random insn.
- UNIMPLEMENTED("Only types are supported");
+ UNREACHABLE("%s", OpcodeName(insn.opcode()).c_str());
return 0;
}
}
@@ -1290,7 +1301,7 @@
break;
}
default:
- UNIMPLEMENTED("%s", OpcodeName(type.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(type.opcode()).c_str());
}
}
@@ -1397,8 +1408,7 @@
d->InsideMatrix = true;
break;
default:
- UNIMPLEMENTED("Unexpected type '%s' in ApplyDecorationsForAccessChain",
- OpcodeName(type.definition.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(type.definition.opcode()).c_str());
}
}
}
@@ -1498,7 +1508,7 @@
break;
}
default:
- UNIMPLEMENTED("Unexpected type '%s' in WalkExplicitLayoutAccessChain", OpcodeName(type.definition.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(type.definition.opcode()).c_str());
}
}
@@ -1547,7 +1557,7 @@
auto &obj = getObject(indexIds[i]);
if (obj.kind != Object::Kind::Constant)
{
- UNIMPLEMENTED("Nonconstant indexing of descriptor arrays is not supported");
+ UNSUPPORTED("SPIR-V SampledImageArrayDynamicIndexing Capability");
}
auto d = descriptorDecorations.at(baseId);
@@ -1575,7 +1585,7 @@
}
default:
- UNIMPLEMENTED("Unexpected type '%s' in WalkAccessChain", OpcodeName(type.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(type.opcode()).c_str());
}
}
@@ -1620,7 +1630,7 @@
}
default:
- UNIMPLEMENTED("Unexpected type in WalkLiteralAccessChain");
+ UNREACHABLE("%s", OpcodeName(type.opcode()).c_str());
}
}
@@ -2437,7 +2447,7 @@
return EmitCopyMemory(insn, state);
default:
- UNIMPLEMENTED("%s", OpcodeName(opcode).c_str());
+ UNREACHABLE("%s", OpcodeName(opcode).c_str());
break;
}
@@ -2513,7 +2523,7 @@
break;
}
default:
- UNIMPLEMENTED("Storage class %d", objectTy.storageClass);
+ UNREACHABLE("Storage class %d", objectTy.storageClass);
break;
}
@@ -3138,7 +3148,7 @@
break;
}
default:
- UNIMPLEMENTED("%s", OpcodeName(insn.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(insn.opcode()).c_str());
}
}
@@ -3334,7 +3344,7 @@
dst.move(i + lhsType.sizeInComponents, MulHigh(lhs.UInt(i), rhs.UInt(i)));
break;
default:
- UNIMPLEMENTED("%s", OpcodeName(insn.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(insn.opcode()).c_str());
}
}
@@ -4177,14 +4187,12 @@
}
case GLSLstd450PackDouble2x32:
{
- // Requires Float64 capability.
- UNIMPLEMENTED("GLSLstd450PackDouble2x32");
+ UNSUPPORTED("SPIR-V Float64 Capability (GLSLstd450PackDouble2x32)");
break;
}
case GLSLstd450UnpackDouble2x32:
{
- // Requires Float64 capability.
- UNIMPLEMENTED("GLSLstd450UnpackDouble2x32");
+ UNSUPPORTED("SPIR-V Float64 Capability (GLSLstd450UnpackDouble2x32)");
break;
}
case GLSLstd450FindILsb:
@@ -4218,20 +4226,17 @@
}
case GLSLstd450InterpolateAtCentroid:
{
- // Requires sampleRateShading / InterpolationFunction capability.
- UNIMPLEMENTED("GLSLstd450InterpolateAtCentroid");
+ UNSUPPORTED("SPIR-V SampleRateShading Capability (GLSLstd450InterpolateAtCentroid)");
break;
}
case GLSLstd450InterpolateAtSample:
{
- // Requires sampleRateShading / InterpolationFunction capability.
- UNIMPLEMENTED("GLSLstd450InterpolateAtSample");
+ UNSUPPORTED("SPIR-V SampleRateShading Capability (GLSLstd450InterpolateAtCentroid)");
break;
}
case GLSLstd450InterpolateAtOffset:
{
- // Requires sampleRateShading / InterpolationFunction capability.
- UNIMPLEMENTED("GLSLstd450InterpolateAtOffset");
+ UNSUPPORTED("SPIR-V SampleRateShading Capability (GLSLstd450InterpolateAtCentroid)");
break;
}
case GLSLstd450NMin:
@@ -4267,7 +4272,7 @@
break;
}
default:
- UNIMPLEMENTED("Unhandled ExtInst %d", extInstIndex);
+ UNREACHABLE("ExtInst %d", int(extInstIndex));
break;
}
@@ -4727,7 +4732,7 @@
break;
}
default:
- UNIMPLEMENTED("EmitImageQuerySize image descriptorType: %d", int(bindingLayout.descriptorType));
+ UNREACHABLE("Image descriptorType: %d", int(bindingLayout.descriptorType));
}
return EmitResult::Continue;
@@ -4996,7 +5001,8 @@
dst.move(3, SIMD::Float(1));
break;
default:
- UNIMPLEMENTED("");
+ UNIMPLEMENTED("spv::ImageFormat %d", int(vkFormat));
+ break;
}
return EmitResult::Continue;
@@ -5085,8 +5091,38 @@
packed[1] = SIMD::UInt(texel.UInt(2) & SIMD::UInt(0xffff)) | (SIMD::UInt(texel.UInt(3) & SIMD::UInt(0xffff)) << 16);
numPackedElements = 2;
break;
+ case spv::ImageFormatRg32f:
+ case spv::ImageFormatRg16f:
+ case spv::ImageFormatR11fG11fB10f:
+ case spv::ImageFormatR16f:
+ case spv::ImageFormatRgba16:
+ case spv::ImageFormatRgb10A2:
+ case spv::ImageFormatRg16:
+ case spv::ImageFormatRg8:
+ case spv::ImageFormatR16:
+ case spv::ImageFormatR8:
+ case spv::ImageFormatRgba16Snorm:
+ case spv::ImageFormatRg16Snorm:
+ case spv::ImageFormatRg8Snorm:
+ case spv::ImageFormatR16Snorm:
+ case spv::ImageFormatR8Snorm:
+ case spv::ImageFormatRg32i:
+ case spv::ImageFormatRg16i:
+ case spv::ImageFormatRg8i:
+ case spv::ImageFormatR16i:
+ case spv::ImageFormatR8i:
+ case spv::ImageFormatRgb10a2ui:
+ case spv::ImageFormatRg32ui:
+ case spv::ImageFormatRg16ui:
+ case spv::ImageFormatRg8ui:
+ case spv::ImageFormatR16ui:
+ case spv::ImageFormatR8ui:
+ UNIMPLEMENTED("spv::ImageFormat %d", int(format));
+ break;
+
default:
- UNIMPLEMENTED("spv::ImageFormat %u", format);
+ UNREACHABLE("spv::ImageFormat %d", int(format));
+ break;
}
auto basePtr = SIMD::Pointer(imageBase, imageSizeInBytes);
@@ -5198,7 +5234,7 @@
v = ExchangeAtomic(Pointer<UInt>(&ptr.base[offset]), laneValue, memoryOrder);
break;
default:
- UNIMPLEMENTED("%s", OpcodeName(insn.opcode()).c_str());
+ UNREACHABLE("%s", OpcodeName(insn.opcode()).c_str());
break;
}
x = Insert(x, v, j);
diff --git a/src/Pipeline/SpirvShaderSampling.cpp b/src/Pipeline/SpirvShaderSampling.cpp
index 7fcd8a0..6c546e5 100644
--- a/src/Pipeline/SpirvShaderSampling.cpp
+++ b/src/Pipeline/SpirvShaderSampling.cpp
@@ -218,7 +218,7 @@
case VK_IMAGE_VIEW_TYPE_CUBE:
break;
case VK_IMAGE_VIEW_TYPE_CUBE_ARRAY:
- UNSUPPORTED("ImageCubeArray");
+ UNSUPPORTED("SPIR-V ImageCubeArray Capability (imageViewType: %d)", int(imageViewType));
if(coordinateIndex == 3)
{
return ADDRESSING_LAYER;
@@ -251,7 +251,7 @@
case VK_IMAGE_VIEW_TYPE_CUBE:
return ADDRESSING_SEAMLESS;
// case VK_IMAGE_VIEW_TYPE_CUBE_ARRAY:
- UNSUPPORTED("ImageCubeArray");
+ UNSUPPORTED("SPIR-V ImageCubeArray Capability (imageViewType: %d)", int(imageViewType));
return ADDRESSING_SEAMLESS;
case VK_IMAGE_VIEW_TYPE_1D:
case VK_IMAGE_VIEW_TYPE_2D:
diff --git a/tests/regres/main.go b/tests/regres/main.go
index 867b881..973e635 100644
--- a/tests/regres/main.go
+++ b/tests/regres/main.go
@@ -1120,7 +1120,7 @@
// Regular expression to parse a test that failed due to UNIMPLEMENTED()
unimplementedRE = regexp.MustCompile(`[^\n]*UNIMPLEMENTED:[^\n]*`)
// Regular expression to parse a test that failed due to UNSUPPORTED()
- unsupportedRE = regexp.MustCompile(`[^\n]*UNSUPPORTED\([^\)]*\):[^\n]*`)
+ unsupportedRE = regexp.MustCompile(`[^\n]*UNSUPPORTED:[^\n]*`)
// Regular expression to parse a test that failed due to UNREACHABLE()
unreachableRE = regexp.MustCompile(`[^\n]*UNREACHABLE:[^\n]*`)
// Regular expression to parse a test that failed due to ASSERT()
