[MLIR][Arith] Add CeilFloorDivExpandOpsPatterns to conversion to LLVM (Reland)

[nujaa]

When running convert-to-llvm, ceildiv and floordiv ops which do not have direct llvm conversion pattern, would not get lowered to llvm dialect. This patch adds CeilFloorDivExpandOpsPatterns to both convert-to-llvm and arith-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 :

• ArithToLLVM needs a new dependency to ArithUtils
• ArithUtils needs new dependency to ArithTransforms or move the patterns as well which will create more dependencies
• It creates lots of code motion which makes it hard to review.

[nujaa]

@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.
It is my first time dealing with the buildbots, I have read about buildbot ownership, buildbot creations, ... but, I cannot access the buildbot builds (e.g. https://lab.llvm.org/buildbot/#/builders/80/builds/7168) . I tried with various networks and browsers, it looks like there is no data. Do I need to ask for specific project authorization ?

[RoboTux]

@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. It is my first time dealing with the buildbots, I have read about buildbot ownership, buildbot creations, ... but, I cannot access the buildbot builds (e.g. https://lab.llvm.org/buildbot/#/builders/80/builds/7168) . I tried with various networks and browsers, it looks like there is no data. Do I need to ask for specific project authorization ?

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.

[banach-space]

I cannot access the buildbot builds (e.g. https://lab.llvm.org/buildbot/#/builders/80/builds/7168) . I tried with various networks and browsers, it looks like there is no data. Do I need to ask for specific project authorization ?

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 flang-aarch64-sharedlibs, which is defined in llvm-zorg here. You can use that definition to see what CMake flags are used for this config. For build failures like this you will very likely discover that adding:

• -DBUILD_SHARED_LIBS=ON

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

[nujaa]

Thank you both. Indeed, -DBUILD_SHARED_LIBS=ON did the trick, I'll definitely add it to my upstream LLVM config. I can now say this patch solves the linkage error of the previous MR. 👍
I'll look into buildbot access for the future.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Hugo Trachino (nujaa)

Changes

When running convert-to-llvm, ceildiv and floordiv ops which do not have direct llvm conversion pattern, would not get lowered to llvm dialect. This patch adds CeilFloorDivExpandOpsPatterns to both convert-to-llvm and arith-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. @RoboTux suggested as well to move the CeilFloorDivExpandOpsPatterns to ArithUtils but I think linking ArithTransforms makes more sense as otherwise :

• ArithToLLVM needs a new dependency to ArithUtils
• ArithUtils needs new dependency to ArithTransforms or move the patterns as well which will create more dependencies
• It creates lots of code motion which makes it hard to review.

---

Full diff: https://github.com/llvm/llvm-project/pull/118839.diff

3 Files Affected:

• (modified) mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp (+4-1)
• (modified) mlir/lib/Conversion/ArithToLLVM/CMakeLists.txt (+1)
• (modified) mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir (+62)

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

[banach-space]

LGTM, thanks!

From what I can tell, Mehdi's concerns have also been addressed, so this should be good to land.

[nujaa]

Should we wait for @joker-eph 's to check whether his concerns have been addressed ?

[banach-space]

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 :-)

[nujaa]

After the previous breakage, I try not to make hasty assumptions. 🙄

[joker-eph]

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).
(Sorry I missed you PR earlier)