Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Negative numerators to modulo/remainder operations are not supported by the Vulkan API. While the SPIR-V instructions
OpSRem and OpSMod have identical semantics to tir::Mod and tir::FloorMod, respectively, use of either instruction within Vulkan results in undefined behavior. From the Vulkan spec:

For the OpSRem and OpSMod instructions, if either operand is negative the result is undefined.

Note: While the OpSRem and OpSMod instructions are supported by the Vulkan environment, they require non-negative values and thus do not enable additional functionality beyond what OpUMod provides.

This issue was first noticed in #13530, where use of integer arithmetic resulted in negative numerators. This hadn't caused issues previously, because most use of div/mod use a denominator that is a power of two. In these cases, tir.LowerIntrin implements floordiv and floormod using only bitwise operations. When the denominator isn't a power of two, both tir::FloorDiv and tir::FloorMod are implemented in terms of tir::Mod, which triggers the undefined behavior for negative numerators.

This commit implements additional simplification rules that preferentially removes negative values from the numerators. For example, simplifying floormod(i - 2, 8) to floormod(i + 6, 8), and simplifying floordiv(i - 2, 8) to floordiv(i + 6, 8) - 1. These handle the most common case, where some index variable is being offset by a negative constant.

Negative numerators to modulo/remainder operations are not supported
by the Vulkan API.  While the SPIR-V instructions
[`OpSRem`](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpSRem)
and
[`OpSMod`](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpSMod)
have identical semantics to `tir::Mod` and `tir::FloorMod`,
respectively, use of either instruction within Vulkan results in
undefined behavior.  From the [Vulkan
spec](https://registry.khronos.org/vulkan/specs/1.3/html/chap37.html#spirvenv-op-prec):

> For the OpSRem and OpSMod instructions, if either operand is
> negative the result is undefined.
>
> Note: While the OpSRem and OpSMod instructions are supported by the
> Vulkan environment, they require non-negative values and thus do not
> enable additional functionality beyond what OpUMod provides.

This issue was first noticed in
apache#13530, where use of integer
arithmetic resulted in negative numerators.  This hadn't caused issues
previously, because most use of div/mod use a denominator that is a
power of two.  In these cases, `tir.LowerIntrin` implements floordiv
and floormod using only bitwise operations.  When the denominator
isn't a power of two, both `tir::FloorDiv` and `tir::FloorMod` are
implemented in terms of `tir::Mod`, which triggers the undefined
behavior for negative numerators.

This commit implements additional simplification rules that
preferentially removes negative values from the numerators.  For
example, simplifying `floormod(i - 2, 8)` to `floormod(i + 6, 8)`, and
simplifying `floordiv(i - 2, 8)` to `floordiv(i + 6, 8) - 1`.  These
handle the most common case, where some index variable is being offset
by a negative constant.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 5, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@Lunderberg
Copy link
Contributor Author

When implementing this change, I had also considered making a Vulkan-specific pass, rather than changing the simplification. However, the effect of such a pass could be undone by a later simplification, so I believe this to be the better solution.

@Lunderberg
Copy link
Contributor Author

The latest change to the simplification caused more breakages than I think it is worth. Closing this PR in favor of #13724, which has a more limited scope.

@Lunderberg Lunderberg closed this Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants