Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
Hitting assert/crash when trying to fold specialization constant
Running any CTS test family `dEQP-VK.spirv_assembly.instruction.graphics.opquantize.spec_const_*inf_*` or `dEQP-VK.spirv_assembly.instruction.graphics.opquantize.spec_const_*nan_*` with VVL enabled will result in hitting the following assert: `opt/fold_spec_constant_op_and_composite_pass.cpp:179: spvtools::opt::Instruction* spvtools::opt::FoldSpecConstantOpAndCompositePass::FoldWithInstructionFolder(spvtools::opt::Module::inst_iterator*): Assertion 'new_const_inst != nullptr && "Failed to fold instruction that must be folded."' failed.`. If run in Release it'll crash.

This can be triggered by running `spirv-opt --freeze-spec-const --fold-spec-const-op-composite -o <output_file> <provided_spirv>`.
Here's an example SPIR-V shader that triggers the assert:
```
OpCapability Shader
OpCapability SignedZeroInfNanPreserve

OpExtension "SPV_KHR_float_controls"

OpMemoryModel Logical GLSL450
OpEntryPoint Vertex %BP_main "main" %BP_stream %BP_position %BP_vtx_color %BP_color %BP_gl_VertexIndex %BP_gl_InstanceIndex  



OpMemberDecorate %BP_gl_PerVertex 0 BuiltIn Position
OpMemberDecorate %BP_gl_PerVertex 1 BuiltIn PointSize
OpMemberDecorate %BP_gl_PerVertex 2 BuiltIn ClipDistance
OpMemberDecorate %BP_gl_PerVertex 3 BuiltIn CullDistance
OpDecorate %BP_gl_PerVertex Block
OpDecorate %BP_position Location 0
OpDecorate %BP_vtx_color Location 1
OpDecorate %BP_color Location 1
OpDecorate %BP_gl_VertexIndex BuiltIn VertexIndex
OpDecorate %BP_gl_InstanceIndex BuiltIn InstanceIndex

OpDecorate %test_constant SpecId 0

%void = OpTypeVoid
%bool = OpTypeBool
%i32 = OpTypeInt 32 1
%u32 = OpTypeInt 32 0
%f32 = OpTypeFloat 32
%v2i32 = OpTypeVector %i32 2
%v2u32 = OpTypeVector %u32 2
%v2f32 = OpTypeVector %f32 2
%v3i32 = OpTypeVector %i32 3
%v3u32 = OpTypeVector %u32 3
%v3f32 = OpTypeVector %f32 3
%v4i32 = OpTypeVector %i32 4
%v4u32 = OpTypeVector %u32 4
%v4f32 = OpTypeVector %f32 4
%v4bool = OpTypeVector %bool 4
%v4f32_v4f32_function = OpTypeFunction %v4f32 %v4f32
%bool_function = OpTypeFunction %bool
%voidf = OpTypeFunction %void
%ip_f32 = OpTypePointer Input %f32
%ip_i32 = OpTypePointer Input %i32
%ip_u32 = OpTypePointer Input %u32
%ip_v2f32 = OpTypePointer Input %v2f32
%ip_v2i32 = OpTypePointer Input %v2i32
%ip_v2u32 = OpTypePointer Input %v2u32
%ip_v3f32 = OpTypePointer Input %v3f32
%ip_v4f32 = OpTypePointer Input %v4f32
%ip_v4i32 = OpTypePointer Input %v4i32
%ip_v4u32 = OpTypePointer Input %v4u32
%op_f32 = OpTypePointer Output %f32
%op_i32 = OpTypePointer Output %i32
%op_u32 = OpTypePointer Output %u32
%op_v2f32 = OpTypePointer Output %v2f32
%op_v2i32 = OpTypePointer Output %v2i32
%op_v2u32 = OpTypePointer Output %v2u32
%op_v4f32 = OpTypePointer Output %v4f32
%op_v4i32 = OpTypePointer Output %v4i32
%op_v4u32 = OpTypePointer Output %v4u32
%fp_f32   = OpTypePointer Function %f32
%fp_i32   = OpTypePointer Function %i32
%fp_v4f32 = OpTypePointer Function %v4f32
%c_f32_1 = OpConstant %f32 1.0
%c_f32_0 = OpConstant %f32 0.0
%c_f32_0_5 = OpConstant %f32 0.5
%c_f32_n1  = OpConstant %f32 -1.
%c_f32_7 = OpConstant %f32 7.0
%c_f32_8 = OpConstant %f32 8.0
%c_i32_0 = OpConstant %i32 0
%c_i32_1 = OpConstant %i32 1
%c_i32_2 = OpConstant %i32 2
%c_i32_3 = OpConstant %i32 3
%c_i32_4 = OpConstant %i32 4
%c_u32_0 = OpConstant %u32 0
%c_u32_1 = OpConstant %u32 1
%c_u32_2 = OpConstant %u32 2
%c_u32_3 = OpConstant %u32 3
%c_u32_32 = OpConstant %u32 32
%c_u32_4 = OpConstant %u32 4
%c_u32_31_bits = OpConstant %u32 0x7FFFFFFF
%c_v4f32_1_1_1_1 = OpConstantComposite %v4f32 %c_f32_1 %c_f32_1 %c_f32_1 %c_f32_1
%c_v4f32_1_0_0_1 = OpConstantComposite %v4f32 %c_f32_1 %c_f32_0 %c_f32_0 %c_f32_1
%c_v4f32_0_5_0_5_0_5_0_5 = OpConstantComposite %v4f32 %c_f32_0_5 %c_f32_0_5 %c_f32_0_5 %c_f32_0_5
%a1f32 = OpTypeArray %f32 %c_u32_1
%a2f32 = OpTypeArray %f32 %c_u32_2
%a3v4f32 = OpTypeArray %v4f32 %c_u32_3
%a4f32 = OpTypeArray %f32 %c_u32_4
%a32v4f32 = OpTypeArray %v4f32 %c_u32_32
%ip_a3v4f32 = OpTypePointer Input %a3v4f32
%ip_a32v4f32 = OpTypePointer Input %a32v4f32
%op_a2f32 = OpTypePointer Output %a2f32
%op_a3v4f32 = OpTypePointer Output %a3v4f32
%op_a4f32 = OpTypePointer Output %a4f32
%BP_gl_PerVertex = OpTypeStruct %v4f32 %f32 %a1f32 %a1f32
%BP_op_gl_PerVertex = OpTypePointer Output %BP_gl_PerVertex
%BP_stream = OpVariable %BP_op_gl_PerVertex Output
%BP_position = OpVariable %ip_v4f32 Input
%BP_vtx_color = OpVariable %op_v4f32 Output
%BP_color = OpVariable %ip_v4f32 Input
%BP_gl_VertexIndex = OpVariable %ip_i32 Input
%BP_gl_InstanceIndex = OpVariable %ip_i32 Input
%test_constant = OpSpecConstant %f32 0.
%c             = OpSpecConstantOp %f32 QuantizeToF16 %test_constant


%BP_main = OpFunction %void None %voidf
%BP_label = OpLabel


%BP_pos = OpLoad %v4f32 %BP_position
%BP_gl_pos = OpAccessChain %op_v4f32 %BP_stream %c_i32_0
OpStore %BP_gl_pos %BP_pos
%BP_col = OpLoad %v4f32 %BP_color
%BP_col_transformed = OpFunctionCall %v4f32 %test_code %BP_col
OpStore %BP_vtx_color %BP_col_transformed
OpReturn
OpFunctionEnd

%isUniqueIdZero = OpFunction %bool None %bool_function
%getId_label = OpLabel
%vert_id = OpLoad %i32 %BP_gl_VertexIndex
%is_id_0 = OpIEqual %bool %vert_id %c_i32_0
OpReturnValue %is_id_0
OpFunctionEnd
%test_code     = OpFunction %v4f32 None %v4f32_v4f32_function
%param1        = OpFunctionParameter %v4f32
%label_testfun = OpLabel
%gz = OpFOrdLessThan %bool %c %c_f32_0
%inf = OpIsInf %bool %c
%cond = OpLogicalAnd %bool %gz %inf

%v4cond        = OpCompositeConstruct %v4bool %cond %cond %cond %cond
%retval        = OpSelect %v4f32 %v4cond %c_v4f32_1_0_0_1 %param1
                 OpReturnValue %retval
OpFunctionEnd
```

It would be good not to crash when something cannot be folded.


Discussion / hints:
removing `OpCapability SignedZeroInfNanPreserve` will have it not crash

----

the fold logic is done in the function pointer returned from `FoldQuantizeToF16Scalar`... but adding some `printf` I see it is never actually invoked in the 

```c++
for (auto rule : GetConstantFoldingRules().GetRulesForInstruction(inst)) {
    folded_const = rule(context_, inst, constants);
```

logic

instead it gets to `FoldFPUnaryOp` and fails in the

```c++
if (!inst->IsFloatingPointFoldingAllowed()) {
    return nullptr;
}
```

call, which propagates all the badness that causes things to fail

Yeah, I think this sums it up: https://github.com/KhronosGroup/SPIRV-Tools/blob/0d87845532579ec3787165cc55635c95180463e0/source/opt/instruction.cpp#L804

Can we return `nullptr` at https://github.com/KhronosGroup/SPIRV-Tools/blob/0d87845532579ec3787165cc55635c95180463e0/source/opt/fold_spec_constant_op_and_composite_pass.cpp#L179 if `new_const_inst` is null? Since there are known unsupported cases, it seems like we should, rather than crash.
agree,  I rather first not crash over anything
@s-perron if this sounds like an acceptable change, I can put up a PR.
Yes, please open a PR.

Fix patch:
diff --git a/source/opt/fold_spec_constant_op_and_composite_pass.cpp b/source/opt/fold_spec_constant_op_and_composite_pass.cpp
index f6d61554a4..770e1fe50c 100644
--- a/source/opt/fold_spec_constant_op_and_composite_pass.cpp
+++ b/source/opt/fold_spec_constant_op_and_composite_pass.cpp
@@ -176,8 +176,9 @@ Instruction* FoldSpecConstantOpAndCompositePass::FoldWithInstructionFolder(
   Instruction* new_const_inst =
       context()->get_instruction_folder().FoldInstructionToConstant(
           inst.get(), identity_map);
-  assert(new_const_inst != nullptr &&
-         "Failed to fold instruction that must be folded.");
+
+  // new_const_inst == null indicates we cannot fold this spec constant
+  if (!new_const_inst) return nullptr;
 
   // Get the instruction before |pos| to insert after.  |pos| cannot be the
   // first instruction in the list because its type has to come first.


Test patch:
diff --git a/test/opt/fold_spec_const_op_composite_test.cpp b/test/opt/fold_spec_const_op_composite_test.cpp
index f83e86e961..70ba362056 100644
--- a/test/opt/fold_spec_const_op_composite_test.cpp
+++ b/test/opt/fold_spec_const_op_composite_test.cpp
@@ -674,6 +674,31 @@ TEST_F(FoldSpecConstantOpAndCompositePassBasicTest, CompositeInsertMatrixNull) {
   SinglePassRunAndMatch<FoldSpecConstantOpAndCompositePass>(test, false);
 }
 
+// Silently ignore spec constants that cannot be folded
+TEST_F(FoldSpecConstantOpAndCompositePassBasicTest, UnfoldableOp) {
+  const std::string test = R"(
+               OpCapability Shader
+               OpCapability SignedZeroInfNanPreserve
+               OpExtension "SPV_KHR_float_controls"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Vertex %main "main"
+               OpSource GLSL 450
+               OpDecorate %v SpecId 1
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+       %float = OpTypeFloat 32
+          %v = OpConstant %float 0x1p-1
+%c = OpSpecConstantOp %float QuantizeToF16 %v
+;CHECK: {{%\w+}} = OpSpecConstantOp {{%\w+}} QuantizeToF16 {{%\w+}}
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+               OpReturn
+               OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<FoldSpecConstantOpAndCompositePass>(test, false);
+}
+
 // All types and some common constants that are potentially required in
 // FoldSpecConstantOpAndCompositeTest.
 std::vector<std::string> CommonTypesAndConstants() {