Skip to content

[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

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

nujaa
Copy link
Contributor

@nujaa nujaa commented Dec 5, 2024

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 added 2 commits December 6, 2024 00:13
Arith Floor and Ceil ops would not get lowered when running
--convert-arith-to-llvm.
@nujaa
Copy link
Contributor Author

nujaa commented Dec 5, 2024

@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
Copy link
Contributor

RoboTux commented Dec 5, 2024

@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
Copy link
Contributor

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
Copy link
Contributor Author

nujaa commented Dec 6, 2024

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.

@nujaa nujaa marked this pull request as ready for review December 6, 2024 10:04
@llvmbot llvmbot added the mlir label Dec 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2024

@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

Copy link
Contributor

@banach-space banach-space left a 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.

@nujaa
Copy link
Contributor Author

nujaa commented Dec 16, 2024

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

@banach-space
Copy link
Contributor

banach-space commented Dec 16, 2024

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
Copy link
Contributor Author

nujaa commented Dec 16, 2024

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

@joker-eph
Copy link
Collaborator

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)

@nujaa nujaa merged commit 3cbc73f into llvm:main Dec 16, 2024
12 checks passed
norx1991 added a commit that referenced this pull request Dec 16, 2024
Fix issue introduced by #118839.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants