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

Issue:
spirv-opt: Crash in Aggressive DCE pass when a buffer reference is used as a return value
See https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5981

This shader will crash when run through ` spirv-opt  --eliminate-dead-code-aggressive`, spirv-val claims it is valid:

```
#version 450
#extension GL_EXT_buffer_reference : require
#extension GL_EXT_scalar_block_layout : require

layout(location = 0) out vec4 color;
layout(buffer_reference, scalar, buffer_reference_align = 4) buffer TestBufferRef {
    uint data;
};
layout(push_constant, scalar) uniform push {
    TestBufferRef test_buffer_ref;
};
TestBufferRef get_ref() {
    return test_buffer_ref;
}
void main() {
    TestBufferRef ref = get_ref();
    color = vec4(1, 0, 1, 1);
}
```


Discussion / hints:
FYI @GabeRundlett @jeremy-lunarg @ncesario-lunarg 
I assume that since this is GLSL, the shader is being run through glslang first, and that the SPIR-V that glslang is generating initially is fine?
@s-perron Can you assign this to @ncesario-lunarg?
I'm still looking at a potential fix on the SPIRV-Tools side (I will probably have to pick that up next week). In the meantime, running `--inline-entry-points-exhaustive` before any of the other passes works around the crash. I've made this change in VVL (https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/6067) in case that's a possible quick fix.

That said, I can only seem to reproduce the crash on the command line with the `spirv-opt` binary, not with this shader inside of VVL.

Fix patch:
diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index 3f982f8856..3d08ec15e0 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/source/opt/aggressive_dead_code_elim_pass.cpp
@@ -438,6 +438,9 @@ std::vector<uint32_t> AggressiveDCEPass::GetLoadedVariablesFromFunctionCall(
     const Instruction* inst) {
   assert(inst->opcode() == spv::Op::OpFunctionCall);
   std::vector<uint32_t> live_variables;
+  // NOTE: we should only be checking function call parameters here, not the
+  // function itself, however, `IsPtr` will trivially return false for
+  // OpFunction
   inst->ForEachInId([this, &live_variables](const uint32_t* operand_id) {
     if (!IsPtr(*operand_id)) return;
     uint32_t var_id = GetVariableId(*operand_id);
diff --git a/source/opt/mem_pass.cpp b/source/opt/mem_pass.cpp
index 9f95785699..9972c4f75f 100644
--- a/source/opt/mem_pass.cpp
+++ b/source/opt/mem_pass.cpp
@@ -76,6 +76,11 @@ bool MemPass::IsNonPtrAccessChain(const spv::Op opcode) const {
 bool MemPass::IsPtr(uint32_t ptrId) {
   uint32_t varId = ptrId;
   Instruction* ptrInst = get_def_use_mgr()->GetDef(varId);
+  if (ptrInst->opcode() == spv::Op::OpFunction) {
+    // A function is not a pointer, but it's return type could be, which will
+    // erroneously lead to this function returning true later on
+    return false;
+  }
   while (ptrInst->opcode() == spv::Op::OpCopyObject) {
     varId = ptrInst->GetSingleWordInOperand(kCopyObjectOperandInIdx);
     ptrInst = get_def_use_mgr()->GetDef(varId);


Test patch:
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index 83aab3c46a..f8f15b6cc4 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/test/opt/aggressive_dead_code_elim_test.cpp
@@ -7884,6 +7884,71 @@ TEST_F(AggressiveDCETest, RemoveWhenUsingPrintfExtension) {
   SinglePassRunAndMatch<AggressiveDCEPass>(text, true);
 }
 
+TEST_F(AggressiveDCETest, FunctionReturnPointer) {
+  // Run DCE when a function returning a pointer to a reference is present
+
+  const std::string text = R"(
+               OpCapability Shader
+               OpCapability PhysicalStorageBufferAddresses
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel PhysicalStorageBuffer64 GLSL450
+               OpEntryPoint Vertex %2 "main" %3 %4
+               OpSource GLSL 450
+               OpSourceExtension "GL_EXT_buffer_reference"
+               OpSourceExtension "GL_EXT_scalar_block_layout"
+               OpName %4 "color"
+               OpMemberDecorate %5 0 Offset 0
+               OpDecorate %5 Block
+               OpMemberDecorate %7 0 Offset 0
+               OpDecorate %7 Block
+               OpDecorate %8 AliasedPointer
+               OpDecorate %4 Location 0
+          %9 = OpTypeVoid
+         %10 = OpTypeFunction %9
+               OpTypeForwardPointer %11 PhysicalStorageBuffer
+         %12 = OpTypeInt 32 0
+          %5 = OpTypeStruct %12
+         %11 = OpTypePointer PhysicalStorageBuffer %5
+;CHECK:         [[pt:%\w+]] = OpTypePointer PhysicalStorageBuffer {{%\w+}}
+         %13 = OpTypeFunction %11
+;CHECK:  [[pt_fn:%\w+]] = OpTypeFunction [[pt]]
+          %7 = OpTypeStruct %11
+         %14 = OpTypePointer PushConstant %7
+          %3 = OpVariable %14 PushConstant
+         %15 = OpTypeInt 32 1
+         %16 = OpConstant %15 0
+         %17 = OpTypePointer PushConstant %11
+         %18 = OpTypePointer Function %11
+         %19 = OpTypeFloat 32
+         %20 = OpTypeVector %19 4
+         %21 = OpTypePointer Output %20
+          %4 = OpVariable %21 Output
+         %22 = OpConstant %19 1
+         %23 = OpConstant %19 0
+         %24 = OpConstantComposite %20 %22 %23 %22 %22
+          %6 = OpFunction %11 None %13
+;CHECK:   [[fn:%\w+]] = OpFunction [[pt]] None [[pt_fn]]
+         %27 = OpLabel
+         %28 = OpAccessChain %17 %3 %16
+         %29 = OpLoad %11 %28
+               OpReturnValue %29
+               OpFunctionEnd
+          %2 = OpFunction %9 None %10
+         %25 = OpLabel
+          %8 = OpVariable %18 Function
+         %26 = OpFunctionCall %11 %6
+;CHECK:  {{%\w+}} = OpFunctionCall [[pt]] [[fn]]
+               OpStore %8 %26
+               OpStore %4 %24
+               OpReturn
+               OpFunctionEnd
+)";
+
+  // For physical storage buffer support
+  SetTargetEnv(SPV_ENV_VULKAN_1_2);
+  SinglePassRunAndMatch<AggressiveDCEPass>(text, true);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools