[Arith] Use ConstIntBound to remove negative numerator when lowering #13724

Lunderberg · 2023-01-07T04:03:33Z

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 alters the lowering of FloorDiv/FloorMod to TruncDiv/TruncMod, in cases where the denominator is positive, the numerator is sometimes negative, and the range of the numerator is known. In these cases, the FloorDiv/FloorMod is now implemented by offsetting the numerator such that it is always positive.

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 alters the lowering of FloorDiv/FloorMod to TruncDiv/TruncMod, in cases where the denominator is positive, the numerator is sometimes negative, and the range of the numerator is known. In these cases, the FloorDiv/FloorMod is now implemented by offsetting the numerator such that it is always positive.

tvm-bot · 2023-01-07T04:03:36Z

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.

cc @junrushao _{See #10317 for details}

_{Generated by tvm-bot}

wrongtest-intellif · 2023-01-10T03:08:50Z

src/tir/transforms/lower_intrin.cc

+      arith::ConstIntBound const_int_bound = analyzer_->const_int_bound(op->a);
+      if (const_int_bound->min_value != arith::ConstIntBound::kNegInf &&
+          const_int_bound->min_value < 0 &&
+          const_int_bound->min_value > -(1LL << (op->a->dtype.bits() - 1))) {


The meaning of bound -(1LL << (op->a->dtype.bits() - 1))) is sign free or not? Could we use min_value interface of op.h here?

Thank you, and op.h's min_value is exactly what I had been looking for. I had checked whether ConstIntBound::Everything(dtype) was public, but wasn't aware of min_value. PR updated to use min_value instead of doing the calculation here.

wrongtest-intellif · 2023-01-10T04:16:07Z

src/tir/transforms/lower_intrin.cc

+        IntImm min(op->a->dtype, const_int_bound->min_value);
+        PrimExpr ceildiv = truncdiv((op->b - 1) - min, op->b);
+        PrimExpr offset_numerator = analyzer_->Simplify(op->a + op->b * ceildiv);
+        return truncdiv(offset_numerator, op->b) - ceildiv;


IIUC, the rationale is:

a // b => (a + -min - -min) // b => (a + (x*b-y) - (x*b-y)) // b => ( where x is ceildiv(-min, b), x >= 0, y >= 0 ) (a + x*b) // b - x => (a + x*b) / b - x ( since a + x*b >= a + -min >= 0 )

Right?

Yup, that is the rationale. I've added the derivation in a comment, which I probably should have done from the start.

wrongtest-intellif · 2023-01-10T04:19:01Z

src/tir/transforms/lower_intrin.cc

+        IntImm min(op->a->dtype, const_int_bound->min_value);
+        PrimExpr ceildiv = truncdiv(-min + (op->b - 1), op->b);
+        PrimExpr offset_numerator = analyzer_->Simplify(op->a + op->b * ceildiv);
+        return truncmod(offset_numerator, op->b);


floormod(a, b) => floormod(a + -min - -min, b) => floormod(a + (x*b-y) - (x*b-y)), b) => ( where x is ceildiv(-min, b), x >= 0, y >= 0 ) floormod(a + x*b, b) => (a + x*b) % b ( since a + x*b >= a + -min >= 0 )

Right?

Yup, that derivation is correct, and I've added a comment here as well.

* [TOPI] Making test_strided_set require a GPU for testing Skipping test_strided_set with ci_cpu docker image due to an issue reported in #13724.

…pache#13724) * [Arith] Use ConstIntBound to remove negative numerator when lowering 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 alters the lowering of FloorDiv/FloorMod to TruncDiv/TruncMod, in cases where the denominator is positive, the numerator is sometimes negative, and the range of the numerator is known. In these cases, the FloorDiv/FloorMod is now implemented by offsetting the numerator such that it is always positive. * Add check to avoid -INT32_MIN * Updated to use `tvm::min_value(DataType)` * Added derivation for floordiv/floormod in terms of truncdiv/trundmod

* [TOPI] Making test_strided_set require a GPU for testing Skipping test_strided_set with ci_cpu docker image due to an issue reported in apache#13724.

Lunderberg mentioned this pull request Jan 7, 2023

[Arith] Simplify to positive numerators in floordiv/floormod #13708

Closed

Add check to avoid -INT32_MIN

2d0a7cf

Lunderberg force-pushed the offset_negative_floormod_floordiv branch from 2fd620f to 2d0a7cf Compare January 10, 2023 02:30

wrongtest-intellif approved these changes Jan 10, 2023

View reviewed changes

Lunderberg added 2 commits January 10, 2023 08:11

Updated to use tvm::min_value(DataType)

a56f5c3

Added derivation for floordiv/floormod in terms of truncdiv/trundmod

c392b3e

Lunderberg merged commit 68c917d into apache:main Jan 10, 2023

This was referenced Jan 18, 2023

[TOPI] Making test_strided_set require a GPU for testing #13804

Merged

[CI Problem] TOPI tests on ci_cpu fail #13805

Closed

asparkhi pushed a commit that referenced this pull request Jan 18, 2023

[TOPI] Making test_strided_set require a GPU for testing (#13804)

5d2a947

* [TOPI] Making test_strided_set require a GPU for testing Skipping test_strided_set with ci_cpu docker image due to an issue reported in #13724.

ysh329 mentioned this pull request Apr 17, 2023

[Release] v0.12.0 Release Candidate Notes #14645

Closed

Lunderberg deleted the offset_negative_floormod_floordiv branch May 9, 2023 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Arith] Use ConstIntBound to remove negative numerator when lowering #13724

[Arith] Use ConstIntBound to remove negative numerator when lowering #13724

Uh oh!

Lunderberg commented Jan 7, 2023

Uh oh!

tvm-bot commented Jan 7, 2023

Uh oh!

wrongtest-intellif Jan 10, 2023

Uh oh!

Lunderberg Jan 10, 2023

Uh oh!

wrongtest-intellif Jan 10, 2023

Uh oh!

Lunderberg Jan 10, 2023

Uh oh!

wrongtest-intellif Jan 10, 2023

Uh oh!

Lunderberg Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Arith] Use ConstIntBound to remove negative numerator when lowering #13724

[Arith] Use ConstIntBound to remove negative numerator when lowering #13724

Uh oh!

Conversation

Lunderberg commented Jan 7, 2023

Uh oh!

tvm-bot commented Jan 7, 2023

Uh oh!

wrongtest-intellif Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Lunderberg Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

wrongtest-intellif Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Lunderberg Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

wrongtest-intellif Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Lunderberg Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants