Skip to content
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][OpenMP] Implement the ConvertToLLVMPatternInterface #101997

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

fabianmcg
Copy link
Contributor

This patch implements the ConvertToLLVMPatternInterface for the OpenMP dialect, allowing convert-to-llvm to act on the OpenMP dialect.

@fabianmcg fabianmcg marked this pull request as ready for review August 5, 2024 15:07
@fabianmcg fabianmcg requested a review from skatrak August 5, 2024 15:08
@llvmbot
Copy link
Member

llvmbot commented Aug 5, 2024

@llvm/pr-subscribers-mlir-openmp
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-flang-openmp

Author: Fabian Mora (fabianmcg)

Changes

This patch implements the ConvertToLLVMPatternInterface for the OpenMP dialect, allowing convert-to-llvm to act on the OpenMP dialect.


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

4 Files Affected:

  • (modified) mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h (+5)
  • (modified) mlir/include/mlir/InitAllExtensions.h (+2)
  • (modified) mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp (+29)
  • (modified) mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir (+1)
diff --git a/mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h b/mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
index 2ed077f8f83e9..808586074c536 100644
--- a/mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
+++ b/mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
@@ -11,6 +11,7 @@
 #include <memory>
 
 namespace mlir {
+class DialectRegistry;
 class LLVMTypeConverter;
 class ConversionTarget;
 class MLIRContext;
@@ -28,6 +29,10 @@ void configureOpenMPToLLVMConversionLegality(ConversionTarget &target,
 /// Populate the given list with patterns that convert from OpenMP to LLVM.
 void populateOpenMPToLLVMConversionPatterns(LLVMTypeConverter &converter,
                                             RewritePatternSet &patterns);
+
+/// Registers the `ConvertToLLVMPatternInterface` interface in the `OpenMP`
+/// dialect.
+void registerConvertOpenMPToLLVMInterface(DialectRegistry &registry);
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_OPENMPTOLLVM_CONVERTOPENMPTOLLVM_H
diff --git a/mlir/include/mlir/InitAllExtensions.h b/mlir/include/mlir/InitAllExtensions.h
index 20a4ab6f18a28..91b3cc160eb62 100644
--- a/mlir/include/mlir/InitAllExtensions.h
+++ b/mlir/include/mlir/InitAllExtensions.h
@@ -22,6 +22,7 @@
 #include "mlir/Conversion/MathToLLVM/MathToLLVM.h"
 #include "mlir/Conversion/MemRefToLLVM/MemRefToLLVM.h"
 #include "mlir/Conversion/NVVMToLLVM/NVVMToLLVM.h"
+#include "mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h"
 #include "mlir/Conversion/UBToLLVM/UBToLLVM.h"
 #include "mlir/Dialect/Affine/TransformOps/AffineTransformOps.h"
 #include "mlir/Dialect/Bufferization/TransformOps/BufferizationTransformOps.h"
@@ -64,6 +65,7 @@ inline void registerAllExtensions(DialectRegistry &registry) {
   registerConvertMathToLLVMInterface(registry);
   registerConvertMemRefToLLVMInterface(registry);
   registerConvertNVVMToLLVMInterface(registry);
+  registerConvertOpenMPToLLVMInterface(registry);
   ub::registerConvertUBToLLVMInterface(registry);
 
   // Register all transform dialect extensions.
diff --git a/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp b/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
index f6a6d1d7228a0..68d4e1b34d065 100644
--- a/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
+++ b/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
@@ -10,6 +10,7 @@
 
 #include "mlir/Conversion/ArithToLLVM/ArithToLLVM.h"
 #include "mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h"
+#include "mlir/Conversion/ConvertToLLVM/ToLLVMInterface.h"
 #include "mlir/Conversion/FuncToLLVM/ConvertFuncToLLVM.h"
 #include "mlir/Conversion/FuncToLLVM/ConvertFuncToLLVMPass.h"
 #include "mlir/Conversion/LLVMCommon/ConversionTarget.h"
@@ -307,3 +308,31 @@ void ConvertOpenMPToLLVMPass::runOnOperation() {
   if (failed(applyPartialConversion(module, target, std::move(patterns))))
     signalPassFailure();
 }
+
+//===----------------------------------------------------------------------===//
+// ConvertToLLVMPatternInterface implementation
+//===----------------------------------------------------------------------===//
+namespace {
+/// Implement the interface to convert OpenMP to LLVM.
+struct OpenMPToLLVMDialectInterface : public ConvertToLLVMPatternInterface {
+  using ConvertToLLVMPatternInterface::ConvertToLLVMPatternInterface;
+  void loadDependentDialects(MLIRContext *context) const final {
+    context->loadDialect<LLVM::LLVMDialect>();
+  }
+
+  /// Hook for derived dialect interface to provide conversion patterns
+  /// and mark dialect legal for the conversion target.
+  void populateConvertToLLVMConversionPatterns(
+      ConversionTarget &target, LLVMTypeConverter &typeConverter,
+      RewritePatternSet &patterns) const final {
+    configureOpenMPToLLVMConversionLegality(target, typeConverter);
+    populateOpenMPToLLVMConversionPatterns(typeConverter, patterns);
+  }
+};
+} // namespace
+
+void mlir::registerConvertOpenMPToLLVMInterface(DialectRegistry &registry) {
+  registry.addExtension(+[](MLIRContext *ctx, omp::OpenMPDialect *dialect) {
+    dialect->addInterfaces<OpenMPToLLVMDialectInterface>();
+  });
+}
diff --git a/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir b/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
index d81487daf34f6..e7cb3d15f861e 100644
--- a/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
+++ b/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
@@ -1,4 +1,5 @@
 // RUN: mlir-opt -convert-openmp-to-llvm -split-input-file %s | FileCheck %s
+// RUN: mlir-opt -convert-to-llvm -split-input-file %s | FileCheck %s
 
 // CHECK-LABEL: llvm.func @foo(i64, i64)
 func.func private @foo(index, index)

@kiranchandramohan
Copy link
Contributor

@fabianmcg This would need an RFC. And you might want to tag Mehdi and Alex Zinenko.

@fabianmcg
Copy link
Contributor Author

fabianmcg commented Aug 5, 2024

@kiranchandramohan I can do an RFC.
However, I didn't think there was a need because this is a pretty small change. Many upstream dialects already implement this interface, for example: arith, complex, cf, func, index, nvvm, math, memref, ub. Also, many dialects that don't have it, is simply because no one has implemented it.

This interface simply removes the necessity of invoking convert-openmp-to-llvm in production pipelines and allows using instead convert-to-llvm.

@fabianmcg fabianmcg requested review from joker-eph and ftynse August 5, 2024 16:24
@kiranchandramohan
Copy link
Contributor

Thanks for the details @fabianmcg. The only question is whether there will be a confusion here because other conversions (of arith, memref etc) fully convert to LLVM dialect whereas OpenMP only converts the types and retains the OpenMP operations.

@fabianmcg
Copy link
Contributor Author

The only question is whether there will be a confusion here because other conversions (of arith, memref etc) fully convert to LLVM dialect whereas OpenMP only converts the types and retains the OpenMP operations.

Not likely, this interface only picks up upstream OpenMP to LLVM patterns (the exact same patterns as ConvertOpenMPToLLVM), hence users shouldn't expect different behavior.

I'll also say that this is a pretty overridable change. For example, if one day a downstream user creates explicit lowerings to LLVM, they can implement the ToLLVM interface and substitute the registration call with theirs and the convert-to-llvm pass would use the downstream implementation. What I'm trying to say, is that this change is pretty localized to upstream MLIR.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. Please wait a couple of days before you submit.

@kiranchandramohan
Copy link
Contributor

reverse ping.

This patch implements the `ConvertToLLVMPatternInterface` for the OpenMP
dialect, allowing `convert-to-llvm` to act on the OpenMP dialect.
@fabianmcg
Copy link
Contributor Author

fabianmcg commented Oct 11, 2024

Apologies @kiranchandramohan I was busy with school and missed the approval. I rebased the PR just to check it again (locally everything seemed to pass). As soon as it passes I'll merge it.

@fabianmcg fabianmcg merged commit 58d9703 into llvm:main Oct 11, 2024
8 checks passed
@fabianmcg fabianmcg deleted the pr-omp-to-llvm branch October 11, 2024 19:41
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
This patch implements the `ConvertToLLVMPatternInterface` for the OpenMP
dialect, allowing `convert-to-llvm` to act on the OpenMP dialect.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
This patch implements the `ConvertToLLVMPatternInterface` for the OpenMP
dialect, allowing `convert-to-llvm` to act on the OpenMP dialect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants