KhronosGroup / KhronosGroup/SPIRV-Cross

[MSL] SPIRV-Cross generates undefined behaviour for OpSMod

Open
#1,933 1 comment 0 reactions 0 assignees View on GitHub
needs triage
Dominant language
GLSL
Stars
2.5k
Forks
713
Avg merge
2d 18h
Merged PRs (30d)
16

Description

Given the following shader that computes the mod by 32 of a number provided as an input:

```
; Magic: 0x07230203 (SPIR-V)
; Version: 0x00010300 (Version: 1.3.0)
; Generator: 0x00220001 (SPIRVSmith)
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %struct_t Block
OpDecorate %struct_variable DescriptorSet 0
OpDecorate %struct_variable Binding 0
OpMemberDecorate %struct_t 0 Offset 0
%void_t = OpTypeVoid
%main_t = OpTypeFunction %void_t
%int_t = OpTypeInt 32 1
%struct_t = OpTypeStruct %int_t
%int_ptr_t = OpTypePointer StorageBuffer %int_t
%struct_ptr_t = OpTypePointer StorageBuffer %struct_t
%const_mod_operand = OpConstant %int_t 32
%const_zero = OpConstant %int_t 0
%struct_variable = OpVariable %struct_ptr_t StorageBuffer
%main = OpFunction %void_t DontInline %main_t
%main_label = OpLabel
%struct_element_ptr = OpAccessChain %int_ptr_t %struct_variable %const_zero
%struct_element = OpLoad %int_t %struct_element_ptr
%op_mod = OpSMod %int_t %struct_element %const_mod_operand
OpStore %struct_element_ptr %op_mod
OpReturn
OpFunctionEnd
```

SPIRV-Cross generates the following MSL equivalent:

```
#include
#include

using namespace metal;

struct _2
{
int _m0;
};

kernel void main0(device _2& _3 [[buffer(0)]])
{
_3._m0 %= 32;
}
```

This is undefined if the input is negative, as specified in [MSL Spec Section 3.1.2](https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf). Could we have a safe wrapper for `OpSMod`, similar to the one generated for `OpFMod`?

Note that this is an issue even in the constant case where we don't perform the mod on an operand from the input buffer:

```
; Magic: 0x07230203 (SPIR-V)
; Version: 0x00010300 (Version: 1.3.0)
; Generator: 0x00220001 (SPIRVSmith)
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %struct_t Block
OpDecorate %struct_variable DescriptorSet 0
OpDecorate %struct_variable Binding 0
OpMemberDecorate %struct_t 0 Offset 0
%void_t = OpTypeVoid
%main_t = OpTypeFunction %void_t
%int_t = OpTypeInt 32 1
%struct_t = OpTypeStruct %int_t
%int_ptr_t = OpTypePointer StorageBuffer %int_t
%struct_ptr_t = OpTypePointer StorageBuffer %struct_t
%const_mod_operand1 = OpConstant %int_t 32
%const_mod_operand2 = OpConstant %int_t -22
%const_zero = OpConstant %int_t 0
%struct_variable = OpVariable %struct_ptr_t StorageBuffer
%main = OpFunction %void_t DontInline %main_t
%main_label = OpLabel
%struct_element_ptr = OpAccessChain %int_ptr_t %struct_variable %const_zero
%op_mod = OpSMod %int_t %const_mod_operand1 %const_mod_operand2
OpStore %struct_element_ptr %op_mod
OpReturn
OpFunctionEnd
```

Translates to:

```
#include
#include

using namespace metal;

struct _2
{
int _m0;
};

kernel void main0(device _2& _3 [[buffer(0)]])
{
_3._m0 = (-22) % 32;
}
```

As far as my understanding of the SPIR-V spec goes, taking the mod of a negative number is legal and doesn't produce UB but I'd be happy if anyone can point to me something that says otherwise.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by locating the OpSMod handling in the MSL backend and compare it with the existing OpFMod wrapper. Use the two SPIR-V examples in this issue to check negative input and constant operands; done means generated MSL preserves the defined SPIR-V signed-modulo behavior without undefined operations.

Written by the indexing model from the issue text.

Assessment

Domain
compilers, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.