Local access chain convert: check for negative indexes (#4884)

An access chain instruction interpretes its index operands as signed.
The composite insert and extract instruction interpret their index
operands as unsigned, so it is not possible to represent a negative
number.

This commit adds a check to the local-access-chain-convert pass to check
for a negative number in the access chain and to not do the conversion.

Fixes #4856

3 files changed, 37 insertions, 3 deletions

diff --git a/source/opt/local_access_chain_convert_pass.cpp b/source/opt/local_access_chain_convert_pass.cpp
index 9491798e4..d11682f34 100644
--- a/source/opt/local_access_chain_convert_pass.cpp
+++ b/source/opt/local_access_chain_convert_pass.cpp
@@ -188,7 +188,9 @@ bool LocalAccessChainConvertPass::Is32BitConstantIndexAccessChain(
       if (opInst->opcode() != SpvOpConstant) return false;
       const auto* index =
           context()->get_constant_mgr()->GetConstantFromInst(opInst);
-      if (index->GetSignExtendedValue() > UINT32_MAX) return false;
+      int64_t index_value = index->GetSignExtendedValue();
+      if (index_value > UINT32_MAX) return false;
+      if (index_value < 0) return false;
     }
     ++inIdx;
     return true;
diff --git a/source/opt/local_access_chain_convert_pass.h b/source/opt/local_access_chain_convert_pass.h
index eabf8645b..c3731b1c9 100644
--- a/source/opt/local_access_chain_convert_pass.h
+++ b/source/opt/local_access_chain_convert_pass.h
@@ -94,8 +94,8 @@ class LocalAccessChainConvertPass : public MemPass {
   bool ReplaceAccessChainLoad(const Instruction* address_inst,
                               Instruction* original_load);
 
-  // Return true if all indices of access chain |acp| are OpConstant integers
-  // whose values can fit into an unsigned 32-bit value.
+  // Return true if all indices of the access chain |acp| are OpConstant
+  // integers whose signed values can be represented as unsigned 32-bit values.
   bool Is32BitConstantIndexAccessChain(const Instruction* acp) const;
 
   // Identify all function scope variables of target type which are
diff --git a/test/opt/local_access_chain_convert_test.cpp b/test/opt/local_access_chain_convert_test.cpp
index 6f5021c47..07fb537c2 100644
--- a/test/opt/local_access_chain_convert_test.cpp
+++ b/test/opt/local_access_chain_convert_test.cpp
@@ -1316,6 +1316,38 @@ OpFunctionEnd
   SinglePassRunAndCheck<LocalAccessChainConvertPass>(assembly, assembly, false,
                                                      true);
 }
+
+TEST_F(LocalAccessChainConvertTest, NegativeIndex) {
+  // The access chain has a negative index and should not be converted because
+  // the extract instruction cannot hold a negative number.
+  const std::string assembly =
+      R"(OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %2 "main"
+OpExecutionMode %2 OriginUpperLeft
+%void = OpTypeVoid
+%4 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%uint = OpTypeInt 32 0
+%uint_3808428041 = OpConstant %uint 3808428041
+%_arr_int_uint_3808428041 = OpTypeArray %int %uint_3808428041
+%_ptr_Function__arr_int_uint_3808428041 = OpTypePointer Function %_arr_int_uint_3808428041
+%_ptr_Function_int = OpTypePointer Function %int
+%int_n1272971256 = OpConstant %int -1272971256
+%2 = OpFunction %void None %4
+%12 = OpLabel
+%13 = OpVariable %_ptr_Function__arr_int_uint_3808428041 Function
+%14 = OpAccessChain %_ptr_Function_int %13 %int_n1272971256
+%15 = OpLoad %int %14
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndCheck<LocalAccessChainConvertPass>(assembly, assembly, false,
+                                                     true);
+}
+
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    Assorted vector and matrix types