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

[CIR] Initial implementation of lowering CIR to MLIR #127835

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andykaylor
Copy link
Contributor

Add support for lowering CIR to more general MLIR and emitting an MLIR text file, including a new command-line option for emitting MLIR via ClangIR. This does not yet support lowering to LLVM IR or other target-specific formats, but that is the ultimate goal, which is why the newly added directory is named "ThroughMLIR".

The intent here is to allow MLIR optimizations that don't recognize the ClangIR dialect to be run before LLVM IR is generated.

Lowering of global pointers to MLIR is not yet supported (here or in the incubator). A test is added here to track that.

Add support for lowering CIR to MLIR and emitting an MLIR text file.

Lowering of global pointers is not yet supported.
@llvmbot llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Feb 19, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 19, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Andy Kaylor (andykaylor)

Changes

Add support for lowering CIR to more general MLIR and emitting an MLIR text file, including a new command-line option for emitting MLIR via ClangIR. This does not yet support lowering to LLVM IR or other target-specific formats, but that is the ultimate goal, which is why the newly added directory is named "ThroughMLIR".

The intent here is to allow MLIR optimizations that don't recognize the ClangIR dialect to be run before LLVM IR is generated.

Lowering of global pointers to MLIR is not yet supported (here or in the incubator). A test is added here to track that.


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

14 Files Affected:

  • (modified) clang/include/clang/CIR/CIRGenerator.h (+1)
  • (modified) clang/include/clang/CIR/FrontendAction/CIRGenAction.h (+8)
  • (modified) clang/include/clang/CIR/LowerToLLVM.h (+4)
  • (modified) clang/include/clang/Driver/Options.td (+2)
  • (modified) clang/include/clang/Frontend/FrontendOptions.h (+3)
  • (modified) clang/lib/CIR/FrontendAction/CIRGenAction.cpp (+17)
  • (modified) clang/lib/CIR/FrontendAction/CMakeLists.txt (+1)
  • (modified) clang/lib/CIR/Lowering/CMakeLists.txt (+1)
  • (added) clang/lib/CIR/Lowering/ThroughMLIR/CMakeLists.txt (+16)
  • (added) clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp (+201)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+2)
  • (modified) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp (+6)
  • (added) clang/test/CIR/Lowering/ThroughMLIR/global-ptrs.cpp (+29)
  • (added) clang/test/CIR/Lowering/ThroughMLIR/global.cpp (+79)
diff --git a/clang/include/clang/CIR/CIRGenerator.h b/clang/include/clang/CIR/CIRGenerator.h
index 414eba80b88b8..76b662c68615c 100644
--- a/clang/include/clang/CIR/CIRGenerator.h
+++ b/clang/include/clang/CIR/CIRGenerator.h
@@ -55,6 +55,7 @@ class CIRGenerator : public clang::ASTConsumer {
   void Initialize(clang::ASTContext &astContext) override;
   bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
   mlir::ModuleOp getModule() const;
+  mlir::MLIRContext &getContext() { return *mlirContext; }
 };
 
 } // namespace cir
diff --git a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
index 99495f4718c5f..1ffffac1719c4 100644
--- a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
+++ b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
@@ -29,6 +29,7 @@ class CIRGenAction : public clang::ASTFrontendAction {
     EmitCIR,
     EmitLLVM,
     EmitBC,
+    EmitMLIR,
     EmitObj,
   };
 
@@ -59,6 +60,13 @@ class EmitCIRAction : public CIRGenAction {
   EmitCIRAction(mlir::MLIRContext *MLIRCtx = nullptr);
 };
 
+class EmitMLIRAction : public CIRGenAction {
+  virtual void anchor();
+
+public:
+  EmitMLIRAction(mlir::MLIRContext *MLIRCtx = nullptr);
+};
+
 class EmitLLVMAction : public CIRGenAction {
   virtual void anchor();
 
diff --git a/clang/include/clang/CIR/LowerToLLVM.h b/clang/include/clang/CIR/LowerToLLVM.h
index 6e1b0270fcd2b..e780a5a747730 100644
--- a/clang/include/clang/CIR/LowerToLLVM.h
+++ b/clang/include/clang/CIR/LowerToLLVM.h
@@ -20,6 +20,7 @@ class Module;
 } // namespace llvm
 
 namespace mlir {
+class MLIRContext;
 class ModuleOp;
 } // namespace mlir
 
@@ -30,6 +31,9 @@ std::unique_ptr<llvm::Module>
 lowerDirectlyFromCIRToLLVMIR(mlir::ModuleOp mlirModule,
                              llvm::LLVMContext &llvmCtx);
 } // namespace direct
+
+mlir::ModuleOp lowerFromCIRToMLIR(mlir::ModuleOp mlirModule,
+                                  mlir::MLIRContext &mlirCtx);
 } // namespace cir
 
 #endif // CLANG_CIR_LOWERTOLLVM_H
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 5ad187926e710..c8509e6f1529f 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2958,6 +2958,8 @@ defm clangir : BoolFOption<"clangir",
   BothFlags<[], [ClangOption, CC1Option], "">>;
 def emit_cir : Flag<["-"], "emit-cir">, Visibility<[ClangOption, CC1Option]>,
   Group<Action_Group>, HelpText<"Build ASTs and then lower to ClangIR">;
+def emit_cir_mlir : Flag<["-"], "emit-cir-mlir">, Visibility<[CC1Option]>, Group<Action_Group>,
+  HelpText<"Build ASTs and then lower through ClangIR to MLIR, emit the .milr file">;
 /// ClangIR-specific options - END
 
 def flto_EQ : Joined<["-"], "flto=">,
diff --git a/clang/include/clang/Frontend/FrontendOptions.h b/clang/include/clang/Frontend/FrontendOptions.h
index 99b2d9a98ed1f..e15ce247d1e6e 100644
--- a/clang/include/clang/Frontend/FrontendOptions.h
+++ b/clang/include/clang/Frontend/FrontendOptions.h
@@ -68,6 +68,9 @@ enum ActionKind {
   /// Emit a .cir file
   EmitCIR,
 
+  /// Emit a .mlir file
+  EmitMLIR,
+
   /// Emit a .ll file.
   EmitLLVM,
 
diff --git a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
index 0f686a36b982b..74f795e7e3307 100644
--- a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
+++ b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
@@ -24,6 +24,7 @@ static BackendAction
 getBackendActionFromOutputType(CIRGenAction::OutputType Action) {
   switch (Action) {
   case CIRGenAction::OutputType::EmitCIR:
+  case CIRGenAction::OutputType::EmitMLIR:
     assert(false &&
            "Unsupported output type for getBackendActionFromOutputType!");
     break; // Unreachable, but fall through to report that
@@ -82,6 +83,7 @@ class CIRGenConsumer : public clang::ASTConsumer {
   void HandleTranslationUnit(ASTContext &C) override {
     Gen->HandleTranslationUnit(C);
     mlir::ModuleOp MlirModule = Gen->getModule();
+    mlir::MLIRContext &MlirCtx = Gen->getContext();
     switch (Action) {
     case CIRGenAction::OutputType::EmitCIR:
       if (OutputStream && MlirModule) {
@@ -90,6 +92,15 @@ class CIRGenConsumer : public clang::ASTConsumer {
         MlirModule->print(*OutputStream, Flags);
       }
       break;
+    case CIRGenAction::OutputType::EmitMLIR: {
+      auto LoweredMlirModule = lowerFromCIRToMLIR(MlirModule, MlirCtx);
+      assert(OutputStream && "No output stream when lowering to MLIR!");
+      // FIXME: we cannot roundtrip prettyForm=true right now.
+      mlir::OpPrintingFlags Flags;
+      Flags.enableDebugInfo(/*enable=*/true, /*prettyForm=*/false);
+      LoweredMlirModule->print(*OutputStream, Flags);
+      break;
+    }
     case CIRGenAction::OutputType::EmitLLVM:
     case CIRGenAction::OutputType::EmitBC:
     case CIRGenAction::OutputType::EmitObj:
@@ -124,6 +135,8 @@ getOutputStream(CompilerInstance &CI, StringRef InFile,
     return CI.createDefaultOutputFile(false, InFile, "s");
   case CIRGenAction::OutputType::EmitCIR:
     return CI.createDefaultOutputFile(false, InFile, "cir");
+  case CIRGenAction::OutputType::EmitMLIR:
+    return CI.createDefaultOutputFile(false, InFile, "mlir");
   case CIRGenAction::OutputType::EmitLLVM:
     return CI.createDefaultOutputFile(false, InFile, "ll");
   case CIRGenAction::OutputType::EmitBC:
@@ -155,6 +168,10 @@ void EmitCIRAction::anchor() {}
 EmitCIRAction::EmitCIRAction(mlir::MLIRContext *MLIRCtx)
     : CIRGenAction(OutputType::EmitCIR, MLIRCtx) {}
 
+void EmitMLIRAction::anchor() {}
+EmitMLIRAction::EmitMLIRAction(mlir::MLIRContext *MLIRCtx)
+    : CIRGenAction(OutputType::EmitMLIR, MLIRCtx) {}
+
 void EmitLLVMAction::anchor() {}
 EmitLLVMAction::EmitLLVMAction(mlir::MLIRContext *MLIRCtx)
     : CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {}
diff --git a/clang/lib/CIR/FrontendAction/CMakeLists.txt b/clang/lib/CIR/FrontendAction/CMakeLists.txt
index ac2b857239d07..e22013575b3c9 100644
--- a/clang/lib/CIR/FrontendAction/CMakeLists.txt
+++ b/clang/lib/CIR/FrontendAction/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangCIRFrontendAction
   clangFrontend
   clangCIR
   clangCIRLoweringDirectToLLVM
+  clangCIRLoweringThroughMLIR
   clangCodeGen
   MLIRCIR
   MLIRIR
diff --git a/clang/lib/CIR/Lowering/CMakeLists.txt b/clang/lib/CIR/Lowering/CMakeLists.txt
index 95c304ded9183..f720e597ecb0f 100644
--- a/clang/lib/CIR/Lowering/CMakeLists.txt
+++ b/clang/lib/CIR/Lowering/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(DirectToLLVM)
+add_subdirectory(ThroughMLIR)
diff --git a/clang/lib/CIR/Lowering/ThroughMLIR/CMakeLists.txt b/clang/lib/CIR/Lowering/ThroughMLIR/CMakeLists.txt
new file mode 100644
index 0000000000000..81fc9703ff0ae
--- /dev/null
+++ b/clang/lib/CIR/Lowering/ThroughMLIR/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  Core
+  Support
+  )
+
+get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
+
+add_clang_library(clangCIRLoweringThroughMLIR
+  LowerCIRToMLIR.cpp
+
+  DEPENDS
+  LINK_LIBS
+  MLIRIR
+  ${dialect_libs}
+  MLIRCIR
+)
diff --git a/clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp b/clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp
new file mode 100644
index 0000000000000..53ec01a22f781
--- /dev/null
+++ b/clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp
@@ -0,0 +1,201 @@
+//====- LowerCIRToMLIR.cpp - Lowering from CIR to MLIR --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements lowering of CIR operations to MLIR.
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/MemRef/IR/MemRef.h"
+#include "mlir/IR/BuiltinDialect.h"
+#include "mlir/Pass/Pass.h"
+#include "mlir/Pass/PassManager.h"
+#include "mlir/Transforms/DialectConversion.h"
+#include "clang/CIR/Dialect/IR/CIRDialect.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
+#include "clang/CIR/LowerToLLVM.h"
+#include "clang/CIR/MissingFeatures.h"
+#include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Support/TimeProfiler.h"
+
+using namespace cir;
+using namespace llvm;
+
+namespace cir {
+
+struct ConvertCIRToMLIRPass
+    : public mlir::PassWrapper<ConvertCIRToMLIRPass,
+                               mlir::OperationPass<mlir::ModuleOp>> {
+  void getDependentDialects(mlir::DialectRegistry &registry) const override {
+    registry.insert<mlir::BuiltinDialect, mlir::memref::MemRefDialect>();
+  }
+  void runOnOperation() final;
+
+  StringRef getDescription() const override {
+    return "Convert the CIR dialect module to MLIR standard dialects";
+  }
+
+  StringRef getArgument() const override { return "cir-to-mlir"; }
+};
+
+class CIRGlobalOpLowering : public mlir::OpConversionPattern<cir::GlobalOp> {
+public:
+  using OpConversionPattern<cir::GlobalOp>::OpConversionPattern;
+  mlir::LogicalResult
+  matchAndRewrite(cir::GlobalOp op, OpAdaptor adaptor,
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    auto moduleOp = op->getParentOfType<mlir::ModuleOp>();
+    if (!moduleOp)
+      return mlir::failure();
+
+    mlir::OpBuilder b(moduleOp.getContext());
+
+    const auto cirSymType = op.getSymType();
+    assert(!cir::MissingFeatures::convertTypeForMemory());
+    auto convertedType = getTypeConverter()->convertType(cirSymType);
+    if (!convertedType)
+      return mlir::failure();
+    auto memrefType = dyn_cast<mlir::MemRefType>(convertedType);
+    if (!memrefType)
+      memrefType = mlir::MemRefType::get({}, convertedType);
+    // Add an optional alignment to the global memref.
+    assert(!cir::MissingFeatures::opGlobalAlignment());
+    mlir::IntegerAttr memrefAlignment = mlir::IntegerAttr();
+    // Add an optional initial value to the global memref.
+    mlir::Attribute initialValue = mlir::Attribute();
+    std::optional<mlir::Attribute> init = op.getInitialValue();
+    if (init.has_value()) {
+      initialValue =
+          llvm::TypeSwitch<mlir::Attribute, mlir::Attribute>(init.value())
+              .Case<cir::IntAttr>([&](cir::IntAttr attr) {
+                auto rtt = mlir::RankedTensorType::get({}, convertedType);
+                return mlir::DenseIntElementsAttr::get(rtt, attr.getValue());
+              })
+              .Case<cir::FPAttr>([&](cir::FPAttr attr) {
+                auto rtt = mlir::RankedTensorType::get({}, convertedType);
+                return mlir::DenseFPElementsAttr::get(rtt, attr.getValue());
+              })
+              .Default([&](mlir::Attribute attr) {
+                llvm_unreachable("GlobalOp lowering with initial value is not "
+                                 "fully supported yet");
+                return mlir::Attribute();
+              });
+    }
+
+    // Add symbol visibility
+    assert(!cir::MissingFeatures::opGlobalLinkage());
+    std::string symVisibility = "public";
+
+    assert(!cir::MissingFeatures::opGlobalConstant());
+    bool isConstant = false;
+
+    rewriter.replaceOpWithNewOp<mlir::memref::GlobalOp>(
+        op, b.getStringAttr(op.getSymName()),
+        /*sym_visibility=*/b.getStringAttr(symVisibility),
+        /*type=*/memrefType, initialValue,
+        /*constant=*/isConstant,
+        /*alignment=*/memrefAlignment);
+
+    return mlir::success();
+  }
+};
+
+void populateCIRToMLIRConversionPatterns(mlir::RewritePatternSet &patterns,
+                                         mlir::TypeConverter &converter) {
+  patterns.add<CIRGlobalOpLowering>(converter, patterns.getContext());
+}
+
+static mlir::TypeConverter prepareTypeConverter() {
+  mlir::TypeConverter converter;
+  converter.addConversion([&](cir::PointerType type) -> mlir::Type {
+    assert(!cir::MissingFeatures::convertTypeForMemory());
+    mlir::Type ty = converter.convertType(type.getPointee());
+    // FIXME: The pointee type might not be converted (e.g. struct)
+    if (!ty)
+      return nullptr;
+    return mlir::MemRefType::get({}, ty);
+  });
+  converter.addConversion(
+      [&](mlir::IntegerType type) -> mlir::Type { return type; });
+  converter.addConversion(
+      [&](mlir::FloatType type) -> mlir::Type { return type; });
+  converter.addConversion([&](cir::VoidType type) -> mlir::Type { return {}; });
+  converter.addConversion([&](cir::IntType type) -> mlir::Type {
+    // arith dialect ops doesn't take signed integer -- drop cir sign here
+    return mlir::IntegerType::get(
+        type.getContext(), type.getWidth(),
+        mlir::IntegerType::SignednessSemantics::Signless);
+  });
+  converter.addConversion([&](cir::SingleType type) -> mlir::Type {
+    return mlir::Float32Type::get(type.getContext());
+  });
+  converter.addConversion([&](cir::DoubleType type) -> mlir::Type {
+    return mlir::Float64Type::get(type.getContext());
+  });
+  converter.addConversion([&](cir::FP80Type type) -> mlir::Type {
+    return mlir::Float80Type::get(type.getContext());
+  });
+  converter.addConversion([&](cir::LongDoubleType type) -> mlir::Type {
+    return converter.convertType(type.getUnderlying());
+  });
+  converter.addConversion([&](cir::FP128Type type) -> mlir::Type {
+    return mlir::Float128Type::get(type.getContext());
+  });
+  converter.addConversion([&](cir::FP16Type type) -> mlir::Type {
+    return mlir::Float16Type::get(type.getContext());
+  });
+  converter.addConversion([&](cir::BF16Type type) -> mlir::Type {
+    return mlir::BFloat16Type::get(type.getContext());
+  });
+
+  return converter;
+}
+
+void ConvertCIRToMLIRPass::runOnOperation() {
+  auto module = getOperation();
+
+  auto converter = prepareTypeConverter();
+
+  mlir::RewritePatternSet patterns(&getContext());
+
+  populateCIRToMLIRConversionPatterns(patterns, converter);
+
+  mlir::ConversionTarget target(getContext());
+  target.addLegalOp<mlir::ModuleOp>();
+  target.addLegalDialect<mlir::memref::MemRefDialect>();
+  target.addIllegalDialect<cir::CIRDialect>();
+
+  if (failed(applyPartialConversion(module, target, std::move(patterns))))
+    signalPassFailure();
+}
+
+std::unique_ptr<mlir::Pass> createConvertCIRToMLIRPass() {
+  return std::make_unique<ConvertCIRToMLIRPass>();
+}
+
+mlir::ModuleOp lowerFromCIRToMLIR(mlir::ModuleOp mlirModule,
+                                  mlir::MLIRContext &mlirCtx) {
+  llvm::TimeTraceScope scope("Lower CIR To MLIR");
+
+  mlir::PassManager pm(&mlirCtx);
+
+  pm.addPass(createConvertCIRToMLIRPass());
+
+  auto result = !mlir::failed(pm.run(mlirModule));
+  if (!result)
+    llvm::report_fatal_error(
+        "The pass manager failed to lower CIR to MLIR standard dialects!");
+
+  // Now that we ran all the lowering passes, verify the final output.
+  if (mlirModule.verify().failed())
+    llvm::report_fatal_error(
+        "Verification of the final MLIR in standard dialects failed!");
+
+  return mlirModule;
+}
+
+} // namespace cir
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 4eb743acf327f..58fdad192056f 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2740,6 +2740,7 @@ static const auto &getFrontendActionTable() {
       {frontend::EmitAssembly, OPT_S},
       {frontend::EmitBC, OPT_emit_llvm_bc},
       {frontend::EmitCIR, OPT_emit_cir},
+      {frontend::EmitMLIR, OPT_emit_cir_mlir},
       {frontend::EmitHTML, OPT_emit_html},
       {frontend::EmitLLVM, OPT_emit_llvm},
       {frontend::EmitLLVMOnly, OPT_emit_llvm_only},
@@ -4631,6 +4632,7 @@ static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   case frontend::EmitAssembly:
   case frontend::EmitBC:
   case frontend::EmitCIR:
+  case frontend::EmitMLIR:
   case frontend::EmitHTML:
   case frontend::EmitLLVM:
   case frontend::EmitLLVMOnly:
diff --git a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
index bb3bb0aac78bf..ec0949c2b6941 100644
--- a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -79,6 +79,12 @@ CreateFrontendBaseAction(CompilerInstance &CI) {
     return std::make_unique<cir::EmitCIRAction>();
 #else
     llvm_unreachable("CIR suppport not built into clang");
+#endif
+  case EmitMLIR:
+#if CLANG_ENABLE_CIR
+    return std::make_unique<cir::EmitMLIRAction>();
+#else
+    llvm_unreachable("CIR suppport not built into clang");
 #endif
   case EmitHTML:               return std::make_unique<HTMLPrintAction>();
   case EmitLLVM: {
diff --git a/clang/test/CIR/Lowering/ThroughMLIR/global-ptrs.cpp b/clang/test/CIR/Lowering/ThroughMLIR/global-ptrs.cpp
new file mode 100644
index 0000000000000..ada6a17896dfa
--- /dev/null
+++ b/clang/test/CIR/Lowering/ThroughMLIR/global-ptrs.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir-mlir %s -o %t.mlir
+// RUN: FileCheck --input-file=%t.mlir %s
+
+// XFAIL: *
+
+// Note: This is here to track the failed lowering of global pointers. When
+//       this test starts passing, the checks should be updated with more
+//       specifics.
+
+void *vp;
+// CHECK: memref.global "public" @vp
+
+int *ip = 0;
+// CHECK: memref.global "public" @ip
+
+double *dp;
+// CHECK: memref.global "public" @dp
+
+char **cpp;
+// CHECK: memref.global "public" @cpp
+
+void (*fp)();
+// CHECK: memref.global "public" @fp
+
+int (*fpii)(int) = 0;
+// CHECK: memref.global "public" @fpii
+
+void (*fpvar)(int, ...);
+// CHECK: memref.global "public" @fpvar
diff --git a/clang/test/CIR/Lowering/ThroughMLIR/global.cpp b/clang/test/CIR/Lowering/ThroughMLIR/global.cpp
new file mode 100644
index 0000000000000..4515d68aa177d
--- /dev/null
+++ b/clang/test/CIR/Lowering/ThroughMLIR/global.cpp
@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir-mlir %s -o %t.mlir
+// RUN: FileCheck --input-file=%t.mlir %s
+
+char c;
+// CHECK: memref.global "public" @c : memref<i8>
+
+signed char sc;
+// CHECK: memref.global "public" @sc : memref<i8>
+
+unsigned char uc;
+// CHECK: memref.global "public" @uc : memref<i8>
+
+short ss;
+// CHECK: memref.global "public" @ss : memref<i16>
+
+unsigned short us = 100;
+// CHECK: memref.global "public" @us : memref<i16> = dense<100>
+
+int si = 42;
+// CHECK: memref.global "public" @si : memref<i32> = dense<42>
+
+unsigned ui;
+// CHECK: memref.global "public" @ui : memref<i32>
+
+long sl;
+// CHECK: memref.global "public" @sl : memref<i64>
+
+unsigned long ul;
+// CHECK: memref.global "public" @ul : memref<i64>
+
+long long sll;
+// CHECK: memref.global "public" @sll : memref<i64>
+
+unsigned long long ull = 123456;
+// CHECK: memref.global "public" @ull : memref<i64> = dense<123456>
+
+__int128 s128;
+// CHECK: memref.global "public" @s128 : memref<i128>
+
+unsigned __int128 u128;
+// CHECK: memref.global "public" @u128 : memref<i128>
+
+wchar_t wc;
+// CHECK: memref.global "public" @wc : memref<i32>
+
+char8_t c8;
+// CHECK: memref.global "public" @c8 : memref<i8>
+
+char16_t c16;
+// CHECK: memref.global "public" @c16 : memref<i16>
+
+char32_t c32;
+// CHECK: memref.global "public" @c32 : memref<i32>
+
+_BitInt(20) sb20;
+// CHECK: memref.global "public" @sb20 : memref<i20>
+
+unsigned _BitInt(48) ub48;
+// CHECK: memref.global "public" @ub48 : memref<i48>
+
+_Float16 f16;
+// CHECK: memref.global "public" @f16 : memref<f16>
+
+__bf16 bf16;
+// CHECK: memref.global "public" @bf16 : memref<bf16>
+
+float f;
+// CHECK: memref.global "public" @f : memref<f32>
+
+double d = 1.25;
+// CHECK: memref.global "public" @d : memref<f64> = dense<1.250000e+00>
+
+long double ld;
+// CHECK: memref.global "public" @ld : memref<f80>
+
+__float128 f128;
+// CHECK: memref.global "public" @f128 : memref<f128>
+
+// FIXME: Add global pointers when they can be lowered to MLIR

@@ -2958,6 +2958,8 @@ defm clangir : BoolFOption<"clangir",
BothFlags<[], [ClangOption, CC1Option], "">>;
def emit_cir : Flag<["-"], "emit-cir">, Visibility<[ClangOption, CC1Option]>,
Group<Action_Group>, HelpText<"Build ASTs and then lower to ClangIR">;
def emit_cir_mlir : Flag<["-"], "emit-cir-mlir">, Visibility<[CC1Option]>, Group<Action_Group>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the incubator this is just "-emit-mlir" but that's claimed below by flang as an alias for "-emit-fir". The incubator just has that line commented out. I find it a bit disturbing that flang is affecting the namespace for clang command-line options, but I guess it's good to have some alignment between the two drivers.

I'm not entirely happy with the name I used here, as it seems to imply the opposite of what it is. It's emitting MLIR with no ClangIR dialect, but I'm not sure how to represent that without a very long name. Suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This IS a shame... IMO, we should bring in the Fortran folks (@clementval for suggestions?) and see if we can make this arrange a way to make this useful in both compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. One problem with aligning the two is that in flang, currently, -emit-mlir means to emit the fir dialect, whereas here we wanted it to mean, emit MLIR but not the cir dialect. This highlights the problem that "emit MLIR" isn't very specific. Even the interpretation used in the ClangIR incubator is emitting a handful of different dialects (affine, scf, arith, memref, vector, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we have currently two options. -emit-fir and -emit-hlfir. These are dump the IR at different level. But as Andy mentioned, it is with the fir or hlfir dialect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a perhaps a term that MEANS 'generic dialects' or something that we could use here instead?

perhaps emit-std-mlir ? And point out th at this is intending to lower to the standard dialects?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a lowering to core dialects at the moment. At least two attempts have been made for that but nothing is upstream so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked in flang and we have a couple of tests (7 files) using that option but we can switch these to use -emit-fir or -emit-hlfir.

@banach-space Since you are probably the initial author of this option, do you have any objection to make -emit-mlir identical for clang and flang?

Copy link
Contributor

Choose a reason for hiding this comment

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

No objections. In fact, -emit-mlir is a misnomer and emit_mlir_EQ would make more sense to me.

Does flang have the ability to lower to MLIR core dialects? I think that's what we're saying we'd like -emit-mlir with no argument to do in clang.

The semantics in Flang and Clang can differ. Whenever that's the case, please use HelpTextForVariant in Options.td to document that.

That said,-emit-mlir is very ambiguous - there's no single MLIR representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add emit_mlir_EQ will that peacefully coexist with the def emit_mlir : Flag<["-"], "emit-mlir">, Alias<emit_fir>; definition until we can update flang to align the behavior of the option with no argument?

Answering my own question, yes, it will peacefully coexist. At least, the clang part of it worked. I can't see a reason that flang would be impacted (functionally) by what I did here, but I didn't build and test flang.

I think it makes sense to put emit_mlir_EQ in here and then have a separate PR to have flang make use of this option, adding the additional flang dialects, and flang-specific help text.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to put emit_mlir_EQ in here and then have a separate PR to have flang make use of this option, adding the additional flang dialects, and flang-specific help text.

Yeah we can address that in a follow up PR if it doesn't break flang with this one.

@@ -55,6 +55,7 @@ class CIRGenerator : public clang::ASTConsumer {
void Initialize(clang::ASTContext &astContext) override;
bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
mlir::ModuleOp getModule() const;
mlir::MLIRContext &getContext() { return *mlirContext; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mlir::MLIRContext &getContext() { return *mlirContext; }
mlir::MLIRContext &getMLIRContext() { return *mlirContext; }

MINOR preference? We have all sorts of contexts all over the place that these are incredibly confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable. I frequently find myself wondering which context things are returning (recently it's been the AST context coming up during IR generation).

Copy link
Contributor

Choose a reason for hiding this comment

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

A little while back I changed all the variable, parameter, and field names in the ClangIR code to be astContext or mlirContext, getting rid of all generic context or cxt names. I didn't do the same for function names. But it would be good if those were also changed.

@@ -2958,6 +2958,8 @@ defm clangir : BoolFOption<"clangir",
BothFlags<[], [ClangOption, CC1Option], "">>;
def emit_cir : Flag<["-"], "emit-cir">, Visibility<[ClangOption, CC1Option]>,
Group<Action_Group>, HelpText<"Build ASTs and then lower to ClangIR">;
def emit_cir_mlir : Flag<["-"], "emit-cir-mlir">, Visibility<[CC1Option]>, Group<Action_Group>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This IS a shame... IMO, we should bring in the Fortran folks (@clementval for suggestions?) and see if we can make this arrange a way to make this useful in both compilers.

@@ -90,6 +92,15 @@ class CIRGenConsumer : public clang::ASTConsumer {
MlirModule->print(*OutputStream, Flags);
}
break;
case CIRGenAction::OutputType::EmitMLIR: {
auto LoweredMlirModule = lowerFromCIRToMLIR(MlirModule, MlirCtx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please spell out the type.

Comment on lines 57 to 59
const auto cirSymType = op.getSymType();
assert(!cir::MissingFeatures::convertTypeForMemory());
auto convertedType = getTypeConverter()->convertType(cirSymType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please spell out the types...


pm.addPass(createConvertCIRToMLIRPass());

auto result = !mlir::failed(pm.run(mlirModule));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please spell out the type.

@@ -55,6 +55,7 @@ class CIRGenerator : public clang::ASTConsumer {
void Initialize(clang::ASTContext &astContext) override;
bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
mlir::ModuleOp getModule() const;
mlir::MLIRContext &getMLIRContext() { return *mlirContext; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

One overall concern I have is that we're adding an entirely new interface to Clang where we could be introducing const-correctness from the start... but we're not doing it. That's not specific to this PR, but it is something I think should be addressed early on. Retrofitting const-correctness is hard and I realize this is touching other interfaces like MLIR and LLVM, but because this is a brand new component, we should be adding const correctness from the start everywhere possible. (IMO, if MLIR lacks const correctness, that's a problem that should be addressed in MLIR given it's also a new interface, comparatively speaking.)

Copy link
Contributor

@xlauko xlauko Feb 20, 2025

Choose a reason for hiding this comment

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

MLIR design is to literally never use const on its primitives: https://mlir.llvm.org/docs/Rationale/UsageOfConst/ This will probably fight back trying to be const correct only on the clang side.

Copy link
Collaborator

@AaronBallman AaronBallman Feb 20, 2025

Choose a reason for hiding this comment

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

Wow, that's disappointing to say the least. My point still stands -- Clang has been striving to improve const correctness over time and this is a new interface where we should not have to try to retrofit const correctness in the future IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so to stay consistent with the MLIR design philosophy, we won't use const with MLIR operations or values, but we should be moving everything else to const-correctness as it is upstreamed. Does that sound reasonable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that sounds like a good path forward. FWIW, I think it's reasonable for CIR to have some ugly const_cast uses to hide the lack of const correctness from Clang, but not to an obnoxious amount (which it sounds like would be the case with MLIR operations or values). Thanks!

@keryell
Copy link
Contributor

keryell commented Feb 22, 2025

Is there any emergency to up-stream this compiler flow compared to just focusing on upstreaming the direct-through-LLVM path first?
My experiments with llvm/clangir#1334 around llvm/clangir#1219 makes me feel that it might be better to spend more time in the incubator.

@andykaylor
Copy link
Contributor Author

Is there any emergency to up-stream this compiler flow compared to just focusing on upstreaming the direct-through-LLVM path first? My experiments with llvm/clangir#1334 around llvm/clangir#1219 makes me feel that it might be better to spend more time in the incubator.

No, there is no urgency. This is just something I started on while waiting for the initial function implementation to land. I'm just trying to open up more possible paths for parallel work in the upstreaming. I agree that lowering to MLIR shouldn't be a priority at this point.

@keryell
Copy link
Contributor

keryell commented Feb 22, 2025

I appreciate the parallelism for sure! 😄
It would be nice if your team can book more time to work on the fundamental issues related to CIR→MLIR in the incubator.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

LGTM once remaining comments from others are addressed.

Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thank you!

@andykaylor
Copy link
Contributor Author

@AaronBallman , @erichkeane I think I've addressed the review comments. Is there anything else you'd like changed?

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

I'm happy with this as is, though note Aaron is away at WG14 in Graz this week. But it looks like you've done everything he's asked for, so feel free to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants