[CIR] Add framework for CIR to LLVM IR lowering #124650

andykaylor · 2025-01-27T23:01:47Z

Create the skeleton framework for lowering from ClangIR to LLVM IR. This initial implementation just creates an empty LLVM IR module. Actual lowering of even minimal ClangIR is deferred to a later patch.

llvmbot · 2025-01-27T23:02:19Z

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: Andy Kaylor (andykaylor)

Changes

Create the skeleton framework for lowering from ClangIR to LLVM IR. This initial implementation just creates an empty LLVM IR module. Actual lowering of even minimal ClangIR is deferred to a later patch.

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

11 Files Affected:

(modified) clang/include/clang/CIR/CIRGenerator.h (+3)
(modified) clang/include/clang/CIR/FrontendAction/CIRGenAction.h (+8)
(added) clang/include/clang/CIR/LowerToLLVM.h (+36)
(modified) clang/lib/CIR/CMakeLists.txt (+1)
(modified) clang/lib/CIR/FrontendAction/CIRGenAction.cpp (+46-14)
(modified) clang/lib/CIR/FrontendAction/CMakeLists.txt (+1)
(added) clang/lib/CIR/Lowering/CMakeLists.txt (+1)
(added) clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt (+8)
(added) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+41)
(modified) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp (+7-1)
(added) clang/test/CIR/Lowering/hello.c (+8)

diff --git a/clang/include/clang/CIR/CIRGenerator.h b/clang/include/clang/CIR/CIRGenerator.h
index 414eba80b88b8f..58ed15041015af 100644
--- a/clang/include/clang/CIR/CIRGenerator.h
+++ b/clang/include/clang/CIR/CIRGenerator.h
@@ -55,6 +55,9 @@ class CIRGenerator : public clang::ASTConsumer {
   void Initialize(clang::ASTContext &astContext) override;
   bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
   mlir::ModuleOp getModule() const;
+  std::unique_ptr<mlir::MLIRContext> takeContext() {
+    return std::move(mlirContext);
+  };
 };
 
 } // namespace cir
diff --git a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
index 2ab612613b73da..5f9110bc83b89f 100644
--- a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
+++ b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h
@@ -26,6 +26,7 @@ class CIRGenAction : public clang::ASTFrontendAction {
 public:
   enum class OutputType {
     EmitCIR,
+    EmitLLVM,
   };
 
 private:
@@ -55,6 +56,13 @@ class EmitCIRAction : public CIRGenAction {
   EmitCIRAction(mlir::MLIRContext *MLIRCtx = nullptr);
 };
 
+class EmitLLVMAction : public CIRGenAction {
+  virtual void anchor();
+
+public:
+  EmitLLVMAction(mlir::MLIRContext *MLIRCtx = nullptr);
+};
+
 } // namespace cir
 
 #endif
diff --git a/clang/include/clang/CIR/LowerToLLVM.h b/clang/include/clang/CIR/LowerToLLVM.h
new file mode 100644
index 00000000000000..82a28270cc1538
--- /dev/null
+++ b/clang/include/clang/CIR/LowerToLLVM.h
@@ -0,0 +1,36 @@
+//====- LowerToLLVM.h- Lowering from CIR to LLVM --------------------------===//
+//
+// 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 declares an interface for converting CIR modules to LLVM IR.
+//
+//===----------------------------------------------------------------------===//
+#ifndef CLANG_CIR_LOWERTOLLVM_H
+#define CLANG_CIR_LOWERTOLLVM_H
+
+#include "mlir/Pass/Pass.h"
+
+#include <memory>
+
+namespace llvm {
+class LLVMContext;
+class Module;
+} // namespace llvm
+
+namespace mlir {
+class ModuleOp;
+} // namespace mlir
+
+namespace cir {
+
+namespace direct {
+std::unique_ptr<llvm::Module> lowerDirectlyFromCIRToLLVMIR(
+    mlir::ModuleOp M, llvm::LLVMContext &Ctx);
+} // namespace direct
+} // namespace cir
+
+#endif // CLANG_CIR_LOWERTOLLVM_H
\ No newline at end of file
diff --git a/clang/lib/CIR/CMakeLists.txt b/clang/lib/CIR/CMakeLists.txt
index f3ef8525e15c26..4a99ecb33dfb23 100644
--- a/clang/lib/CIR/CMakeLists.txt
+++ b/clang/lib/CIR/CMakeLists.txt
@@ -5,3 +5,4 @@ add_subdirectory(Dialect)
 add_subdirectory(CodeGen)
 add_subdirectory(FrontendAction)
 add_subdirectory(Interfaces)
+add_subdirectory(Lowering)
diff --git a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
index 21b6bc56ed0503..583fd8126e9518 100644
--- a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
+++ b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp
@@ -8,8 +8,10 @@
 
 #include "clang/CIR/FrontendAction/CIRGenAction.h"
 #include "clang/CIR/CIRGenerator.h"
+#include "clang/CIR/LowerToLLVM.h"
+#include "clang/CodeGen/BackendUtil.h"
 #include "clang/Frontend/CompilerInstance.h"
-
+#include "llvm/IR/Module.h"
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/IR/OwningOpRef.h"
 
@@ -18,12 +20,30 @@ using namespace clang;
 
 namespace cir {
 
+static BackendAction
+getBackendActionFromOutputType(CIRGenAction::OutputType Action) {
+  switch (Action) {
+  case CIRGenAction::OutputType::EmitLLVM:
+    return BackendAction::Backend_EmitLL;
+  default:
+    llvm_unreachable("Unsupported action");
+  }
+}
+
+static std::unique_ptr<llvm::Module> lowerFromCIRToLLVMIR(
+    const clang::FrontendOptions &FEOpts, mlir::ModuleOp MLIRModule,
+    std::unique_ptr<mlir::MLIRContext> MLIRCtx, llvm::LLVMContext &LLVMCtx) {
+  return direct::lowerDirectlyFromCIRToLLVMIR(MLIRModule, LLVMCtx);
+}
+
 class CIRGenConsumer : public clang::ASTConsumer {
 
   virtual void anchor();
 
   CIRGenAction::OutputType Action;
 
+  CompilerInstance &CI;
+
   std::unique_ptr<raw_pwrite_stream> OutputStream;
 
   ASTContext *Context{nullptr};
@@ -32,17 +52,12 @@ class CIRGenConsumer : public clang::ASTConsumer {
 
 public:
   CIRGenConsumer(CIRGenAction::OutputType Action,
-                 DiagnosticsEngine &DiagnosticsEngine,
-                 IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-                 const HeaderSearchOptions &HeaderSearchOptions,
-                 const CodeGenOptions &CodeGenOptions,
-                 const TargetOptions &TargetOptions,
-                 const LangOptions &LangOptions,
-                 const FrontendOptions &FEOptions,
+                 CompilerInstance &CI,
                  std::unique_ptr<raw_pwrite_stream> OS)
-      : Action(Action), OutputStream(std::move(OS)), FS(VFS),
-        Gen(std::make_unique<CIRGenerator>(DiagnosticsEngine, std::move(VFS),
-                                           CodeGenOptions)) {}
+      : Action(Action), CI(CI), OutputStream(std::move(OS)),
+        FS(&CI.getVirtualFileSystem()),
+        Gen(std::make_unique<CIRGenerator>(CI.getDiagnostics(), std::move(FS),
+                                           CI.getCodeGenOpts())) {}
 
   void Initialize(ASTContext &Ctx) override {
     assert(!Context && "initialized multiple times");
@@ -58,6 +73,7 @@ class CIRGenConsumer : public clang::ASTConsumer {
   void HandleTranslationUnit(ASTContext &C) override {
     Gen->HandleTranslationUnit(C);
     mlir::ModuleOp MlirModule = Gen->getModule();
+    auto MLIRCtx = Gen->takeContext();
     switch (Action) {
     case CIRGenAction::OutputType::EmitCIR:
       if (OutputStream && MlirModule) {
@@ -66,6 +82,18 @@ class CIRGenConsumer : public clang::ASTConsumer {
         MlirModule->print(*OutputStream, Flags);
       }
       break;
+    case CIRGenAction::OutputType::EmitLLVM: {
+      llvm::LLVMContext LLVMCtx;
+      auto LLVMModule = lowerFromCIRToLLVMIR(CI.getFrontendOpts(), MlirModule,
+                                             std::move(MLIRCtx), LLVMCtx);
+
+      BackendAction BEAction = getBackendActionFromOutputType(Action);
+      emitBackendOutput(CI, CI.getCodeGenOpts(),
+                        C.getTargetInfo().getDataLayoutString(),
+                        LLVMModule.get(), BEAction, FS,
+                        std::move(OutputStream));
+      break;
+    }
     }
   }
 };
@@ -84,6 +112,8 @@ getOutputStream(CompilerInstance &CI, StringRef InFile,
   switch (Action) {
   case CIRGenAction::OutputType::EmitCIR:
     return CI.createDefaultOutputFile(false, InFile, "cir");
+  case CIRGenAction::OutputType::EmitLLVM:
+    return CI.createDefaultOutputFile(false, InFile, "ll");
   }
   llvm_unreachable("Invalid CIRGenAction::OutputType");
 }
@@ -96,9 +126,7 @@ CIRGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
     Out = getOutputStream(CI, InFile, Action);
 
   auto Result = std::make_unique<cir::CIRGenConsumer>(
-      Action, CI.getDiagnostics(), &CI.getVirtualFileSystem(),
-      CI.getHeaderSearchOpts(), CI.getCodeGenOpts(), CI.getTargetOpts(),
-      CI.getLangOpts(), CI.getFrontendOpts(), std::move(Out));
+      Action, CI, std::move(Out));
 
   return Result;
 }
@@ -106,3 +134,7 @@ CIRGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
 void EmitCIRAction::anchor() {}
 EmitCIRAction::EmitCIRAction(mlir::MLIRContext *MLIRCtx)
     : CIRGenAction(OutputType::EmitCIR, MLIRCtx) {}
+
+void EmitLLVMAction::anchor() {}
+EmitLLVMAction::EmitLLVMAction(mlir::MLIRContext *MLIRCtx)
+    : CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {}
\ No newline at end of file
diff --git a/clang/lib/CIR/FrontendAction/CMakeLists.txt b/clang/lib/CIR/FrontendAction/CMakeLists.txt
index b0616ab5d64b09..d87ff7665987d1 100644
--- a/clang/lib/CIR/FrontendAction/CMakeLists.txt
+++ b/clang/lib/CIR/FrontendAction/CMakeLists.txt
@@ -12,6 +12,7 @@ add_clang_library(clangCIRFrontendAction
   clangAST
   clangFrontend
   clangCIR
+  clangCIRLoweringDirectToLLVM
   MLIRCIR
   MLIRIR
   )
diff --git a/clang/lib/CIR/Lowering/CMakeLists.txt b/clang/lib/CIR/Lowering/CMakeLists.txt
new file mode 100644
index 00000000000000..44ec87c28d65e3
--- /dev/null
+++ b/clang/lib/CIR/Lowering/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(DirectToLLVM)
\ No newline at end of file
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt b/clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt
new file mode 100644
index 00000000000000..0268234c3a2896
--- /dev/null
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt
@@ -0,0 +1,8 @@
+set(LLVM_LINK_COMPONENTS
+  Core
+  Support
+  )
+
+add_clang_library(clangCIRLoweringDirectToLLVM
+  LowerToLLVM.cpp
+  )
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
new file mode 100644
index 00000000000000..4bf0f3ab78c53d
--- /dev/null
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -0,0 +1,41 @@
+//====- LowerToLLVM.cpp - Lowering from CIR to LLVMIR ---------------------===//
+//
+// 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 LLVMIR.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/CIR/LowerToLLVM.h"
+
+#include "mlir/IR/BuiltinOps.h"
+#include "mlir/Pass/Pass.h"
+#include "mlir/Pass/PassManager.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/TimeProfiler.h"
+
+
+using namespace cir;
+using namespace llvm;
+
+namespace cir {
+namespace direct {
+
+std::unique_ptr<llvm::Module>
+lowerDirectlyFromCIRToLLVMIR(mlir::ModuleOp theModule, LLVMContext &llvmCtx) {
+  llvm::TimeTraceScope scope("lower from CIR to LLVM directly");
+
+  auto ModuleName = theModule.getName();
+  auto llvmModule = std::make_unique<llvm::Module>(ModuleName ? *ModuleName : "CIRToLLVMModule", llvmCtx);
+
+  if (!llvmModule)
+    report_fatal_error("Lowering from LLVMIR dialect to llvm IR failed!");
+
+  return llvmModule;
+}
+} // namespace direct
+} // namespace cir
diff --git a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
index 3f95a1efb2eed7..f947299292a36d 100644
--- a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -72,7 +72,13 @@ CreateFrontendBaseAction(CompilerInstance &CI) {
     llvm_unreachable("CIR suppport not built into clang");
 #endif
   case EmitHTML:               return std::make_unique<HTMLPrintAction>();
-  case EmitLLVM:               return std::make_unique<EmitLLVMAction>();
+  case EmitLLVM: {
+#if CLANG_ENABLE_CIR
+    if (UseCIR)
+      return std::make_unique<cir::EmitLLVMAction>();
+#endif
+    return std::make_unique<EmitLLVMAction>();
+  }
   case EmitLLVMOnly:           return std::make_unique<EmitLLVMOnlyAction>();
   case EmitCodeGenOnly:        return std::make_unique<EmitCodeGenOnlyAction>();
   case EmitObj:                return std::make_unique<EmitObjAction>();
diff --git a/clang/test/CIR/Lowering/hello.c b/clang/test/CIR/Lowering/hello.c
new file mode 100644
index 00000000000000..320041f0ab7dc9
--- /dev/null
+++ b/clang/test/CIR/Lowering/hello.c
@@ -0,0 +1,8 @@
+// Smoke test for ClangIR-to-LLVM IR code generation
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o -  | FileCheck %s
+
+// TODO: Add checks when proper lowering is implemented.
+//       For now, we're just creating an empty module.
+// CHECK: ModuleID
+
+void foo() {}

github-actions · 2025-01-27T23:05:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bcardosolopes

Seems like there are some CI failures (format related?) but LGTM once those are fixed

lanza · 2025-01-27T23:11:47Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+lowerDirectlyFromCIRToLLVMIR(mlir::ModuleOp theModule, LLVMContext &llvmCtx) {
+  llvm::TimeTraceScope scope("lower from CIR to LLVM directly");
+
+  auto ModuleName = theModule.getName();


We need by code style rules for upstream pretty strictly. Almost-never-use-auto is one of them. So expand this out.

Another is to make sure we're using the right casing throughout. TBH I forget which ruleset we use for this file as we have a partial compromise between MLIR in some places and Clang in others, but clang-tidy should be warning you about incorrect casings within llvm-project while upstreaming ClangIR.

https://llvm.github.io/clangir/GettingStarted/coding-guideline.html

lanza · 2025-01-27T23:12:56Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

-                 const TargetOptions &TargetOptions,
-                 const LangOptions &LangOptions,
-                 const FrontendOptions &FEOptions,
+                 CompilerInstance &CI,


Nice, just caught this cleanup while rebasing ClangIR, too.

erichkeane

I have concerns about takeContext, else everything else is a NIT.

erichkeane · 2025-01-28T14:31:47Z

clang/include/clang/CIR/CIRGenerator.h

@@ -55,6 +55,9 @@ class CIRGenerator : public clang::ASTConsumer {
  void Initialize(clang::ASTContext &astContext) override;
  bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
  mlir::ModuleOp getModule() const;
+  std::unique_ptr<mlir::MLIRContext> takeContext() {


This function is somewhat frightning TBH. It potentially leaves the generator in an invalid state. Is there a reason whoever takes this couldn't just keep a reference to this (for vice versa?).

I'm not sure why it was implemented this way, but I agree with you. In the incubator implementation, the class we're taking it from uses it in the Initialize() function and then never again, but in the Initialize() function it passes this as a raw pointer to one of its contained objects, which keeps it throughout its lifetime. So, we need to keep it around for the lifetime of the generator one way or another. I don't think anything will use it after the point that we take it here, but I also don't see any harm in making it safer.

I thinks dates back to early prototypes for CIR and clang-tidy integration, but seems like the final version doesn't need it either, see clang-tools-extra/clang-tidy/cir/Lifetime.cpp. Should be fine to get rid of it.

erichkeane · 2025-01-28T14:32:22Z

clang/include/clang/CIR/LowerToLLVM.h

+} // namespace direct
+} // namespace cir
+
+#endif // CLANG_CIR_LOWERTOLLVM_H


needs end of line at the end of file. Technically no longer a UB, but it makes Github grumpy.

erichkeane · 2025-01-28T14:35:19Z

clang/lib/CIR/Lowering/CMakeLists.txt

@@ -0,0 +1 @@
+add_subdirectory(DirectToLLVM)


newline at end of file here too, because github.

erichkeane · 2025-01-28T18:26:16Z

clang/include/clang/CIR/CIRGenerator.h

@@ -44,7 +44,7 @@ class CIRGenerator : public clang::ASTConsumer {
  const clang::CodeGenOptions &codeGenOpts;

 protected:
-  std::unique_ptr<mlir::MLIRContext> mlirContext;
+  std::shared_ptr<mlir::MLIRContext> mlirContext;


Hmm... I don't think the shared pointer is the way here, we should have one of them own, and the other observe. What are the lifetimes of these two objects? Is there one of these that has a lifetime that is clearly larger than the other that can be the owner, and the other be a reference to it?

@bcardosolopes

Sorry, I thought shared pointer was what you were suggesting.

Here's what I'm seeing. CIRGenerator::Initialize creates the mlirContext and uses it. That function also passes it, as a raw pointer, to CIRGenModule, which keeps a copy of the pointer throughout its lifetime. CIRGenerator doesn't use it again, and was previously handing it off in CIRGenConsumer::handleTranslationUnit() via takeContext(). CIRGenConsumer::handleTranslationUnit(), in turn, passes it to a few calls as a raw pointer (in the incubator, not yet here) before handing it off, as a unique pointer, to lowerFromCIRToLLVMIR() which hands it, as a unique pointer, to lowerFromCIRToMLIRToLLVMIR() in the incubator implementation. There it is used as a raw pointer.

All of the uses after the call to takeContext() have lifetimes that should be limited to the scope of CIRGenConsumer::handleTranslationUnit(), so I think we should be able to just use it as a raw pointer here instead of taking it and leaving CIRGenerator with a null pointer, but CIRGenerator shouldn't need it again either.

@bcardosolopes Have I misunderstood anything here? What's your recommendation?

Perhaps it's also relevant to note that I don't seem to need the context here in the current patch. It's only used as a unique pointer in the path where we're lowering CIR to other MLIR dialects before generating LLVM IR. So, I could remove this part of the change and defer the decision until it is needed.

Could CIRGenerator own this and just share it with lowerFromCIRToLLVMIR/etc via reference? CIRGenerator seems to be the entry point into all of CIR, so it makes sense for me for it to 'own' the lifetime.

I don't see why handleTranslationUnit needs to take ownership of it. I also wonder if that taking of ownership is problematic, since of course, clang can in some configurations handle multiple translation units, which this could potentially lose (though IDK if we ended up enabling that functionality).

Could CIRGenerator own this and just share it with lowerFromCIRToLLVMIR/etc via reference? CIRGenerator seems to be the entry point into all of CIR, so it makes sense for me for it to 'own' the lifetime.

I think hoisting it to the CIRGenConsumer and just letting the CIRGenerator get a handle to it makes sense.

Alternatively, the CIRGenerator is done at this point. We could have some sort of destruction of the CIRGenerator here and have it give up it's state contents to the CIRGenConsumer. cc @bcardosolopes

We run passes post CIRGen (LLVM lowering is one example) and at some point we were trying to build CIR in Sema and hand it over to codegen if already produced, so this probably dates back to that. Right now CIRGenerator holds the whole lifetime it's totally fine to go for a more clean solution here.

Btw, If you want to double check, you could run the cir tests with a ASAN enabled clang build, I usually use that from time to time to make sure the incubator isn't leaking.

clang/include/clang/CIR/FrontendAction/CIRGenAction.h

AaronBallman · 2025-01-28T19:01:16Z

clang/include/clang/CIR/CIRGenerator.h

@@ -55,6 +55,9 @@ class CIRGenerator : public clang::ASTConsumer {
  void Initialize(clang::ASTContext &astContext) override;
  bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
  mlir::ModuleOp getModule() const;
+  std::shared_ptr<mlir::MLIRContext> getContext() {


Should this be a const member function?

AaronBallman · 2025-01-28T19:02:45Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

+  default:
+    llvm_unreachable("Unsupported action");


Should we have an assertion earlier in the function that it's not OutputType::EmitCIR?

There will eventually be three other output types that are handled here and two more that are not. This function is called from within a switch statement that will ensure that the default case here isn't hit. What would be the advantage of an assert over the unreachable call?

It boils down to invariants. There is no BackendAction::Backend_EmitCIR, so passing in EmitCIR as an output type seems like it would always be a mistaken use of the API, so an assert would make more sense, leaving the unreachable for nonsense cases like passing (CIRGenAction::OutputType)12.

That makes sense. When the assertion is hit, would you rather see the code proceed to call llvm_unreachable for non-assert builds or return Backend_EmitMCNull? I'm thinking to make this forward-looking, I'll convert it to a fully-covered switch with no default and assert(false) in the Backend_EmitCIR case. That way when the other output types are added the compiler will force us to add them to the asserting case here.

So

case CIRGenAction::OutputType::EmitCIR: assert(false && "Unsupported output type for getBackendActionFromOutputType!"); return Backend_EmitMCNull;

or

case CIRGenAction::OutputType::EmitCIR: assert(false && "Unsupported output type for getBackendActionFromOutputType!"); llvm_unreachable("Unsuppported output type");

?

That does leave the (CIRGenAction::OutputType)12 case unhandled, but the fully covered enum switch seems more consistent with general LLVM coding standards. So maybe:

switch (Action) { case CIRGenAction::OutputType::EmitCIR: assert(false && "Unsupported output type for getBackendActionFromOutputType!"); break; // Unreachable, but fall through to report that case CIRGenAction::OutputType::EmitLLVM: return BackendAction::Backend_EmitLL; } llvm_unreachable("Unsuppported output type");

?

That does leave the (CIRGenAction::OutputType)12 case unhandled, but the fully covered enum switch seems more consistent with general LLVM coding standards. So maybe

Yup, I think that's a good approach.

AaronBallman · 2025-01-28T19:05:12Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

+static std::unique_ptr<llvm::Module> lowerFromCIRToLLVMIR(
+    const clang::FrontendOptions &FEOpts, mlir::ModuleOp MLIRModule,
+    std::shared_ptr<mlir::MLIRContext> MLIRCtx, llvm::LLVMContext &LLVMCtx) {


Should we assume that FEOpts and MLIRCtx will be used in a follow-up? (Mostly wondering whether the params should be dropped until there's a use of them, otherwise this won't be -Werror clean for folks with -Wunused enabled.)

You're right. I should remove these extra parameters for now.

AaronBallman · 2025-01-28T19:06:27Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

@@ -66,6 +81,17 @@ class CIRGenConsumer : public clang::ASTConsumer {
        MlirModule->print(*OutputStream, Flags);
      }
      break;
+    case CIRGenAction::OutputType::EmitLLVM: {
+      llvm::LLVMContext LLVMCtx;
+      auto LLVMModule = lowerFromCIRToLLVMIR(CI.getFrontendOpts(), MlirModule,


Please spell out the type instead of using auto.

AaronBallman · 2025-01-28T19:08:54Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+#include "mlir/IR/BuiltinOps.h"
+#include "mlir/Pass/Pass.h"
+#include "mlir/Pass/PassManager.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/TimeProfiler.h"
+


Is this from include what you use? (Code might be fine, I'm just surprised at how much is required to be included given the implementation.)

Sorry. I was using more of these an an earlier version in my sandbox, and I didn't clean up the includes after I stripped down the patch.

AaronBallman · 2025-01-28T19:10:22Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+  if (!M)
+    report_fatal_error("Lowering from LLVMIR dialect to llvm IR failed!");


Is this mostly for debugging purposes while we get stuff up and running? The concern here is that CIR is a library interface and reporting a fatal error would be frustrating if the consumer of the library has different ideas about how to handle such a failure.

The incubator implementation uses report_fatal_error in several situations similar to this. I don't know if it was intended as a temporary measure or not. Is the right way to handle this passing in a reference to a DiagnosticsEngine?

"It depends" (sorry for the total non-answer, lol).

If the reason for the failure is because of user error, then we would want to thread in a DiagnosticsEngine so we can report diagnostics the usual way.

If the reason for the failure is because the CIR developer is holding things wrong, then we probably would want an assertion closer to the actual issue, and then silently return null here.

If the reason for the failure leads to "there's no way to recover from this, everything is in a horrible state", then report_fatal_error is more reasonable.

User errors will have been reported before this. I think the expectation is that we shouldn't hit this failure, but while the CIR code is still partially implemented we probably will. For now, this is kind of acting as a backstop against failures. So, returning to your original question, I think, yes, this is "mostly for debugging purposes while we get stuff up and running."

Most of the things using llvmModule in CIR do probably already emit errors before hitting this. However, there are functions outside CIR that take a llvmModule that we don't control but we use (e.g. prepareLLVMModule), and report_fatal_error is an extra mechanism to make sure compilation chokes if something weird comes out of it.

As a temporary measure, this is fine to leave in for now. However, I think this should migrate away from report_fatal_error in the future so that consumers of the library have the freedom to handle failures more gracefully (an assert would be reasonable).

AaronBallman

LGTM!

AaronBallman · 2025-01-30T18:52:40Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+  if (!M)
+    report_fatal_error("Lowering from LLVMIR dialect to llvm IR failed!");


As a temporary measure, this is fine to leave in for now. However, I think this should migrate away from report_fatal_error in the future so that consumers of the library have the freedom to handle failures more gracefully (an assert would be reasonable).

AaronBallman · 2025-01-30T18:55:44Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

+  default:
+    llvm_unreachable("Unsupported action");


That does leave the (CIRGenAction::OutputType)12 case unhandled, but the fully covered enum switch seems more consistent with general LLVM coding standards. So maybe

Yup, I think that's a good approach.

This reverts commit 38ddcb7.

[CIR] Add framework for CIR to LLVM IR lowering

41546e2

Create the skeleton framework for lowering from ClangIR to LLVM IR. This initial implementation just creates an empty LLVM IR module. Actual lowering of even minimal ClangIR is deferred to a later patch.

andykaylor requested review from AaronBallman, bcardosolopes, erichkeane, lanza and dkolsen-pgi January 27, 2025 23:01

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Jan 27, 2025

bcardosolopes approved these changes Jan 27, 2025

View reviewed changes

lanza reviewed Jan 27, 2025

View reviewed changes

Formatting and style changes

a9ea2d8

erichkeane reviewed Jan 28, 2025

View reviewed changes

Make CIRGenerator::mlirContext a shared pointer

dc69628

erichkeane reviewed Jan 28, 2025

View reviewed changes

AaronBallman reviewed Jan 28, 2025

View reviewed changes

Address review feedback

1b0e5c6

AaronBallman approved these changes Jan 30, 2025

View reviewed changes

erichkeane approved these changes Jan 30, 2025

View reviewed changes

andykaylor merged commit 38ddcb7 into llvm:main Jan 30, 2025
6 of 8 checks passed

andykaylor deleted the lower-to-llvm branch March 4, 2025 22:55

bcardosolopes pushed a commit to bcardosolopes/llvm-project that referenced this pull request Mar 6, 2025

Revert "[CIR] Add framework for CIR to LLVM IR lowering (llvm#124650)"

ef4d5a7

This reverts commit 38ddcb7.

		if (!M)
		report_fatal_error("Lowering from LLVMIR dialect to llvm IR failed!");

		@@ -0,0 +1 @@
		add_subdirectory(DirectToLLVM)

[CIR] Add framework for CIR to LLVM IR lowering #124650

[CIR] Add framework for CIR to LLVM IR lowering #124650

Uh oh!

Conversation

andykaylor commented Jan 27, 2025

Uh oh!

llvmbot commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardosolopes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

llvmbot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading