-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[mlir][arith] Match folding of arith.remf
to llvm.frem
semantics
#96537
Conversation
There are multiple ways to define the remainder operation. Depending on the definition, the result is either always positive or has the sign of the dividend. The pattern lowering `arith.remf` to LLVM assumes that the semantics match `llvm.frem`, which seems to be reasonable. The folder, however, is implemented via `APFloat::remainder()` which has different semantics. This patch matches the folding behaviour to lowering behavior by using `APFloat::mod()`, which matches the behavior of `llvm.frem` and libm's `fmod()`. It also updates the documentation of `arith.remf` to explicitly state that it follows the semantics of `llvm.frem`. frem documentation: https://llvm.org/docs/LangRef.html#frem-instruction Fix llvm#94431
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-arith Author: Felix Schneider (ubfx) ChangesThere are multiple ways to define the remainder operation. Depending on the definition, the result is either always positive or has the sign of the dividend. This patch matches the folding behaviour to lowering behavior by using frem documentation: https://llvm.org/docs/LangRef.html#frem-instruction Fix #94431 Full diff: https://github.com/llvm/llvm-project/pull/96537.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
index 6e24b607cebcf..01581869f9cd8 100644
--- a/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
+++ b/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
@@ -1133,6 +1133,10 @@ def Arith_DivFOp : Arith_FloatBinaryOp<"divf"> {
def Arith_RemFOp : Arith_FloatBinaryOp<"remf"> {
let summary = "floating point division remainder operation";
+ let description = [{
+ The `arith.remf` operation returns the floating point division remainder
+ following the semantics of `llvm.frem`.
+ }];
let hasFolder = 1;
}
diff --git a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
index f4781fcb546a3..b926ff008b3ce 100644
--- a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
@@ -1204,7 +1204,9 @@ OpFoldResult arith::RemFOp::fold(FoldAdaptor adaptor) {
return constFoldBinaryOp<FloatAttr>(adaptor.getOperands(),
[](const APFloat &a, const APFloat &b) {
APFloat result(a);
- (void)result.remainder(b);
+ // Mirror behavior of llvm.frem which
+ // behaves like libm's fmod(a, b).
+ (void)result.mod(b);
return result;
});
}
diff --git a/mlir/test/Dialect/Arith/canonicalize.mlir b/mlir/test/Dialect/Arith/canonicalize.mlir
index 4fe7cfb689be8..31ed8b2cafbc0 100644
--- a/mlir/test/Dialect/Arith/canonicalize.mlir
+++ b/mlir/test/Dialect/Arith/canonicalize.mlir
@@ -2465,9 +2465,10 @@ func.func @test_remsi_1(%arg : vector<4xi32>) -> (vector<4xi32>) {
}
// -----
+// llvm.frem: "The remainder has the same sign as the dividend"
// CHECK-LABEL: @test_remf(
-// CHECK: %[[res:.+]] = arith.constant -1.000000e+00 : f32
+// CHECK: %[[res:.+]] = arith.constant 1.000000e+00 : f32
// CHECK: return %[[res]]
func.func @test_remf() -> (f32) {
%v1 = arith.constant 3.0 : f32
@@ -2476,11 +2477,24 @@ func.func @test_remf() -> (f32) {
return %0 : f32
}
+// CHECK-LABEL: @test_remf2(
+// CHECK: %[[respos:.+]] = arith.constant 1.000000e+00 : f32
+// CHECK: %[[resneg:.+]] = arith.constant -1.000000e+00 : f32
+// CHECK: return %[[respos]], %[[resneg]]
+func.func @test_remf2() -> (f32, f32) {
+ %v1 = arith.constant 3.0 : f32
+ %v2 = arith.constant -2.0 : f32
+ %v3 = arith.constant -3.0 : f32
+ %0 = arith.remf %v1, %v2 : f32
+ %1 = arith.remf %v3, %v2 : f32
+ return %0, %1 : f32, f32
+}
+
// CHECK-LABEL: @test_remf_vec(
// CHECK: %[[res:.+]] = arith.constant dense<[1.000000e+00, 0.000000e+00, -1.000000e+00, 0.000000e+00]> : vector<4xf32>
// CHECK: return %[[res]]
func.func @test_remf_vec() -> (vector<4xf32>) {
- %v1 = arith.constant dense<[1.0, 2.0, 3.0, 4.0]> : vector<4xf32>
+ %v1 = arith.constant dense<[1.0, 2.0, -3.0, 4.0]> : vector<4xf32>
%v2 = arith.constant dense<[2.0, 2.0, 2.0, 2.0]> : vector<4xf32>
%0 = arith.remf %v1, %v2 : vector<4xf32>
return %0 : vector<4xf32>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SPIR-V, we have OpFRem
and OpFMod
, with the former matching the sign of the lhs operand, and the latter matching the sign of the rhs operand: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpFRem. The arith to spirv lowering emits spirv.FRem
:
spirv::ElementwiseOpPattern<arith::RemFOp, spirv::FRemOp>, |
I think this is consistent with the updated fold, but could you double-check that this is truly the case?
Thank you for pointing me to this.
This seems to be correct to me:
Rounding towards zero means that abs(ny) <= abs(x) such that sgn(x-ny) = sgn(x). This matches the behavior of OpFRem as well, i.e. "sign matches the sign of Operand 1". Finally, APFloat::mod() also references fmod() and behaves like this. llvm-project/llvm/lib/Support/APFloat.cpp Lines 2226 to 2227 in 44c9a26
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % documentation. Also, please update the PR description:
It also updates the documentation of arith.remf to explicitly state that it follows the semantics of llvm.frem.
Not true anymore
Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
…lvm#96537) There are multiple ways to define a remainder operation. Depending on the definition, the result could be either always positive or have the sign of the dividend. The pattern lowering `arith.remf` to LLVM assumes that the semantics match `llvm.frem`, which seems to be reasonable. The folder, however, is implemented via `APFloat::remainder()` which has different semantics. This patch matches the folding behaviour to lowering behavior by using `APFloat::mod()`, which matches the behavior of `llvm.frem` and libm's `fmod()`. It also updates the documentation of `arith.remf` to explain this behavior: The sign of the result of the remainder operation always matches the sign of the dividend (LHS operand). frem documentation: https://llvm.org/docs/LangRef.html#frem-instruction Fix llvm#94431 --------- Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
There are multiple ways to define a remainder operation. Depending on the definition, the result could be either always positive or have the sign of the dividend.
The pattern lowering
arith.remf
to LLVM assumes that the semantics matchllvm.frem
, which seems to be reasonable. The folder, however, is implemented viaAPFloat::remainder()
which has different semantics.This patch matches the folding behaviour to lowering behavior by using
APFloat::mod()
, which matches the behavior ofllvm.frem
and libm'sfmod()
. It also updates the documentation ofarith.remf
to explain this behavior: The sign of the result of the remainder operation always matches the sign of the dividend (LHS operand).frem documentation: https://llvm.org/docs/LangRef.html#frem-instruction
Fix #94431