[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.
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:
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 |
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.remfto 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.fremand libm'sfmod(). It also updates the documentation ofarith.remfto 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