-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[MLIR][Arith] Add CeilFloorDivExpandOpsPatterns to conversion to LLVM (Reland) #118839
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
Conversation
Arith Floor and Ceil ops would not get lowered when running --convert-arith-to-llvm.
@RoboTux @banach-space , I added a dependence to MLIRTransforms in this patch, thinking it would solve the linkage error. But, since I could not reproduce this linkage error downstream, I am unsure this fix actually solves the linkage. |
One way to reproduce would be to build just lib/libArithToLLVM.a instead of building the whole of MLIR. If that still passes (without your updated patch) you could try to change the build tool (from Ninja to GNU Make or the opposite) to see if that helps. |
This is 100% public and you should be able to access it. Having said that, Buildbot can be slow and sometimes just doesn't load. This particular build logs loads for me just fine right now. That buildbot is
does the trick :) (as in, shared-libs configuration is much more sensitive to missing dependencies). Hope this helps, if not we'll dig a bit deeper 👍🏻 Ah, and here's the actual CMake invoc from the failing buildbot job: cmake -DLLVM_TARGETS_TO_BUILD=AArch64 -DBUILD_SHARED_LIBS=ON -DLLVM_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17 '-DLLVM_ENABLE_PROJECTS=mlir;llvm;clang;flang' -DLLVM_ENABLE_RUNTIMES=openmp -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_LIT_ARGS=-v -vv' -GNinja ../llvm-project/llvm |
Thank you both. Indeed, |
@llvm/pr-subscribers-mlir Author: Hugo Trachino (nujaa) ChangesWhen running convert-to-llvm, Reland of #117305 after buildbot failures. Added dependence to ArithTransforms in ArithToLLVM. @RoboTux suggested as well to move the CeilFloorDivExpandOpsPatterns to ArithUtils but I think linking ArithTransforms makes more sense as otherwise :
Full diff: https://github.com/llvm/llvm-project/pull/118839.diff 3 Files Affected:
diff --git a/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp b/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp
index 54d941ae9f6c89..211921638e42a7 100644
--- a/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp
+++ b/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp
@@ -13,6 +13,7 @@
#include "mlir/Conversion/LLVMCommon/ConversionTarget.h"
#include "mlir/Conversion/LLVMCommon/VectorPattern.h"
#include "mlir/Dialect/Arith/IR/Arith.h"
+#include "mlir/Dialect/Arith/Transforms/Passes.h"
#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
#include "mlir/IR/TypeUtilities.h"
@@ -504,7 +505,8 @@ struct ArithToLLVMConversionPass
options.overrideIndexBitwidth(indexBitwidth);
LLVMTypeConverter converter(&getContext(), options);
- mlir::arith::populateArithToLLVMConversionPatterns(converter, patterns);
+ arith::populateCeilFloorDivExpandOpsPatterns(patterns);
+ arith::populateArithToLLVMConversionPatterns(converter, patterns);
if (failed(applyPartialConversion(getOperation(), target,
std::move(patterns))))
@@ -530,6 +532,7 @@ struct ArithToLLVMDialectInterface : public ConvertToLLVMPatternInterface {
void populateConvertToLLVMConversionPatterns(
ConversionTarget &target, LLVMTypeConverter &typeConverter,
RewritePatternSet &patterns) const final {
+ arith::populateCeilFloorDivExpandOpsPatterns(patterns);
arith::populateArithToLLVMConversionPatterns(typeConverter, patterns);
}
};
diff --git a/mlir/lib/Conversion/ArithToLLVM/CMakeLists.txt b/mlir/lib/Conversion/ArithToLLVM/CMakeLists.txt
index bb1fa2fbb6577a..0a0e25e18b47af 100644
--- a/mlir/lib/Conversion/ArithToLLVM/CMakeLists.txt
+++ b/mlir/lib/Conversion/ArithToLLVM/CMakeLists.txt
@@ -13,6 +13,7 @@ add_mlir_conversion_library(MLIRArithToLLVM
LINK_LIBS PUBLIC
MLIRArithAttrToLLVMConversion
MLIRArithDialect
+ MLIRArithTransforms
MLIRLLVMCommonConversion
MLIRLLVMDialect
)
diff --git a/mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir b/mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir
index 64c40f1aba43bc..a9dcc0a16b3dbd 100644
--- a/mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir
+++ b/mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir
@@ -540,6 +540,68 @@ func.func @select(%arg0 : i1, %arg1 : i32, %arg2 : i32) -> i32 {
// -----
+// CHECK-LABEL: @ceildivsi
+// CHECK-SAME: %[[ARG0:.*]]: i64) -> i64
+func.func @ceildivsi(%arg0 : i64) -> i64 {
+ // CHECK: %[[CST0:.*]] = llvm.mlir.constant(1 : i64) : i64
+ // CHECK: %[[CST1:.*]] = llvm.mlir.constant(0 : i64) : i64
+ // CHECK: %[[CST2:.*]] = llvm.mlir.constant(-1 : i64) : i64
+ // CHECK: %[[CMP0:.*]] = llvm.icmp "sgt" %[[ARG0]], %[[CST1]] : i64
+ // CHECK: %[[SEL0:.*]] = llvm.select %[[CMP0]], %[[CST2]], %[[CST0]] : i1, i64
+ // CHECK: %[[ADD0:.*]] = llvm.add %[[SEL0]], %[[ARG0]] : i64
+ // CHECK: %[[DIV0:.*]] = llvm.sdiv %[[ADD0]], %[[ARG0]] : i64
+ // CHECK: %[[ADD1:.*]] = llvm.add %[[DIV0]], %[[CST0]] : i64
+ // CHECK: %[[SUB0:.*]] = llvm.sub %[[CST1]], %[[ARG0]] : i64
+ // CHECK: %[[DIV1:.*]] = llvm.sdiv %[[SUB0]], %[[ARG0]] : i64
+ // CHECK: %[[SUB1:.*]] = llvm.sub %[[CST1]], %[[DIV1]] : i64
+ // CHECK: %[[CMP1:.*]] = llvm.icmp "slt" %[[ARG0]], %[[CST1]] : i64
+ // CHECK: %[[CMP2:.*]] = llvm.icmp "sgt" %[[ARG0]], %[[CST1]] : i64
+ // CHECK: %[[CMP3:.*]] = llvm.icmp "slt" %[[ARG0]], %[[CST1]] : i64
+ // CHECK: %[[CMP4:.*]] = llvm.icmp "sgt" %[[ARG0]], %[[CST1]] : i64
+ // CHECK: %[[AND0:.*]] = llvm.and %[[CMP1]], %[[CMP3]] : i1
+ // CHECK: %[[AND1:.*]] = llvm.and %[[CMP2]], %[[CMP4]] : i1
+ // CHECK: %[[OR:.*]] = llvm.or %[[AND0]], %[[AND1]] : i1
+ // CHECK: %[[SEL1:.*]] = llvm.select %[[OR]], %[[ADD1]], %[[SUB1]] : i1, i64
+ %0 = arith.ceildivsi %arg0, %arg0 : i64
+ return %0: i64
+}
+
+// CHECK-LABEL: @ceildivui
+// CHECK-SAME: %[[ARG0:.*]]: i32) -> i32
+func.func @ceildivui(%arg0 : i32) -> i32 {
+// CHECK: %[[CST0:.*]] = llvm.mlir.constant(0 : i32) : i32
+// CHECK: %[[CMP0:.*]] = llvm.icmp "eq" %[[ARG0]], %[[CST0]] : i32
+// CHECK: %[[CST1:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK: %[[SUB0:.*]] = llvm.sub %[[ARG0]], %[[CST1]] : i32
+// CHECK: %[[DIV0:.*]] = llvm.udiv %[[SUB0]], %[[ARG0]] : i32
+// CHECK: %[[ADD0:.*]] = llvm.add %[[DIV0]], %[[CST1]] : i32
+// CHECK: %[[SEL0:.*]] = llvm.select %[[CMP0]], %[[CST0]], %[[ADD0]] : i1, i32
+ %0 = arith.ceildivui %arg0, %arg0 : i32
+ return %0: i32
+}
+
+// -----
+
+// CHECK-LABEL: @floordivsi
+// CHECK-SAME: %[[ARG0:.*]]: i32, %[[ARG1:.*]]: i32) -> i32
+func.func @floordivsi(%arg0 : i32, %arg1 : i32) -> i32 {
+ // CHECK: %[[SDIV:.*]] = llvm.sdiv %[[ARG0]], %[[ARG1]] : i32
+ // CHECK: %[[MUL0:.*]] = llvm.mul %[[SDIV]], %[[ARG1]] : i32
+ // CHECK: %[[CMP0:.*]] = llvm.icmp "ne" %[[ARG0]], %[[MUL0]] : i32
+ // CHECK: %[[CST0:.*]] = llvm.mlir.constant(0 : i32) : i32
+ // CHECK: %[[CMP1:.*]] = llvm.icmp "slt" %[[ARG0]], %[[CST0]] : i32
+ // CHECK: %[[CMP2:.*]] = llvm.icmp "slt" %[[ARG1]], %[[CST0]] : i32
+ // CHECK: %[[CMP3:.*]] = llvm.icmp "ne" %[[CMP1]], %[[CMP2]] : i1
+ // CHECK: %[[AND:.*]] = llvm.and %[[CMP0]], %[[CMP3]] : i1
+ // CHECK: %[[CST1:.*]] = llvm.mlir.constant(-1 : i32) : i32
+ // CHECK: %[[ADD:.*]] = llvm.add %[[SDIV]], %[[CST1]] : i32
+ // CHECK: %[[SEL:.*]] = llvm.select %[[AND]], %[[ADD]], %[[SDIV]] : i1, i32
+ %0 = arith.floordivsi %arg0, %arg1 : i32
+ return %0 : i32
+}
+
+// -----
+
// CHECK-LABEL: @minmaxi
func.func @minmaxi(%arg0 : i32, %arg1 : i32) -> i32 {
// CHECK: = llvm.intr.smin(%arg0, %arg1) : (i32, i32) -> i32
|
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, thanks!
From what I can tell, Mehdi's concerns have also been addressed, so this should be good to land.
Should we wait for @joker-eph 's to check whether his concerns have been addressed ? |
As hinted in my approval, IMO all concerns have been addressed and this is ready to land. If Mehdi complains, you can blame me ;-) Having said that, since you've just pinged Mehdi, I'd wait 24hrs :-) |
After the previous breakage, I try not to make hasty assumptions. 🙄 |
That's fine, this kind of breakages are common, and most of the time re-landing with the fix is straightforward and does not need a great deal of reviews (if the fix is trivial, you can re-land without extra reviews). |
Fix issue introduced by #118839.
When running convert-to-llvm,
ceildiv
andfloordiv
ops which do not have direct llvm conversion pattern, would not get lowered to llvm dialect. This patch adds CeilFloorDivExpandOpsPatterns to bothconvert-to-llvm
andarith-to-llvm
(deprecated) lowering those ops to lower level arith ops which can be lowered to llvm using LLVM conversion.Reland of #117305 after buildbot failures.
See:
https://lab.llvm.org/buildbot/#/builders/80/builds/7168
https://lab.llvm.org/buildbot/#/builders/130/builds/7036
https://lab.llvm.org/buildbot/#/builders/138/builds/7290
Added dependence to ArithTransforms in ArithToLLVM. In previous discussion, it has been suggested to move the CeilFloorDivExpandOpsPatterns to ArithUtils but I think linking ArithTransforms makes more sense as otherwise :