-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tosa] : Fix integer overflow for computing intmax+1 in tosa.cast to linalg. #112455
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Sayan Saha (sahas3) ChangesThis PR fixes an issue related to integer overflow when computing Found this issue while debugging a numerical mismatch for Full diff: https://github.com/llvm/llvm-project/pull/112455.diff 2 Files Affected:
diff --git a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
index c88f4db27c304e..e6b3e4b677e4f2 100644
--- a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
+++ b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
@@ -563,7 +563,7 @@ static Value createLinalgBodyCalculationForElementwiseOp(
getElementTypeOrSelf(srcTy),
APInt::getSignedMaxValue(dstTy.getIntOrFloatBitWidth())
.getSExtValue() +
- 1));
+ 1.0));
auto intMax = rewriter.create<arith::ConstantOp>(
loc, rewriter.getIntegerAttr(
diff --git a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir
index f9d37f9427d4f4..7e2ec67d38d378 100644
--- a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir
+++ b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir
@@ -1929,3 +1929,30 @@ func.func @test_dynamic_fft2d(%arg0: tensor<?x?x?xf32>, %arg1: tensor<?x?x?xf32>
%output_real, %output_imag = "tosa.fft2d"(%arg0, %arg1) {inverse = true} : (tensor<?x?x?xf32>, tensor<?x?x?xf32>) -> (tensor<?x?x?xf32>, tensor<?x?x?xf32>)
return %output_real, %output_imag : tensor<?x?x?xf32>, tensor<?x?x?xf32>
}
+
+
+// -----
+
+// CHECK: #[[$MAP0:.+]] = affine_map<(d0) -> (0)>
+// CHECK: #[[$MAP1:.+]] = affine_map<(d0) -> (d0)>
+
+// CHECK-LABEL: func.func @test_cast_fp32_i64(
+// CHECK-SAME: %[[ARG0:.*]]: tensor<1xf32>) -> tensor<1xi64> {
+// CHECK: %[[VAL_0:.*]] = tensor.empty() : tensor<1xi64>
+// CHECK: %[[RESULT:.*]] = linalg.generic {indexing_maps = [#[[$MAP0]], #[[$MAP1]]], iterator_types = ["parallel"]} ins(%[[ARG0]] : tensor<1xf32>) outs(%[[VAL_0]] : tensor<1xi64>) {
+// CHECK: ^bb0(%[[VAL_2:.*]]: f32, %[[VAL_3:.*]]: i64):
+// CHECK: %[[VAL_4:.*]] = math.roundeven %[[VAL_2]] : f32
+// CHECK: %[[VAL_5:.*]] = arith.constant -9.22337203E+18 : f32
+// CHECK: %[[VAL_6:.*]] = arith.constant 9.22337203E+18 : f32
+// CHECK: %[[VAL_7:.*]] = arith.constant 9223372036854775807 : i64
+// CHECK: %[[VAL_8:.*]] = arith.maximumf %[[VAL_4]], %[[VAL_5]] : f32
+// CHECK: %[[VAL_9:.*]] = arith.fptosi %[[VAL_8]] : f32 to i64
+// CHECK: %[[VAL_10:.*]] = arith.cmpf uge, %[[VAL_4]], %[[VAL_6]] : f32
+// CHECK: %[[VAL_11:.*]] = arith.select %[[VAL_10]], %[[VAL_7]], %[[VAL_9]] : i64
+// CHECK: linalg.yield %[[VAL_11]] : i64
+// CHECK: } -> tensor<1xi64>
+// CHECK: return %[[RESULT]] : tensor<1xi64>
+func.func @test_cast_fp32_i64(%arg0: tensor<1xf32>) -> (tensor<1xi64>) {
+ %0 = tosa.cast %arg0 : (tensor<1xf32>) -> tensor<1xi64>
+ return %0: tensor<1xi64>
+}
|
Without the fix the IR after
was
|
@sahas3 IMHO I think is better to fix; my expectation is that at some point TOSA will get Please wait also for @RoboTux to review the patch before merging. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
This PR fixes an issue related to integer overflow when computing
(intmax+1)
fori64
duringtosa-to-linalg
pass fortosa.cast
.Found this issue while debugging a numerical mismatch for
deeplabv3
model fromtorchvision
represented intosa
dialect using theTorchToTosa
pipeline intorch-mlir
repository.torch.aten.to.dtype
is converted totosa.cast
that castsf32
toi64
type. Technically by the specification,tosa.cast
doesn't handle castingf32
toi64
. So it's possible to add a verifier to error out for such tosa ops instead of producing incorrect code. However, I chose to fix the overflow issue to still be able to represent thedeeplabv3
model withtosa
ops in the above-mentioned pipeline. Open to suggestions if adding the verifier is more appropriate instead.