Skip to content

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

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.

Sign up for GitHub

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 4 commits into from
Jan 30, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41546e2
[CIR] Add framework for CIR to LLVM IR lowering
andykaylor Jan 27, 2025
a9ea2d8
Formatting and style changes
andykaylor Jan 28, 2025
dc69628
Make CIRGenerator::mlirContext a shared pointer
andykaylor Jan 28, 2025
1b0e5c6
Address review feedback
andykaylor Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/CIR/CIRGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Collaborator

@erichkeane erichkeane Jan 28, 2025

Choose a reason for hiding this comment

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

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?).

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bcardosolopes bcardosolopes Jan 28, 2025

Choose a reason for hiding this comment

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

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.

return std::move(mlirContext);
};
};

} // namespace cir
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/CIR/FrontendAction/CIRGenAction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class CIRGenAction : public clang::ASTFrontendAction {
public:
enum class OutputType {
EmitCIR,
EmitLLVM,
};

private:
Expand Down Expand Up @@ -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
36 changes: 36 additions & 0 deletions clang/include/clang/CIR/LowerToLLVM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Collaborator

@erichkeane erichkeane Jan 28, 2025

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions clang/lib/CIR/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ add_subdirectory(Dialect)
add_subdirectory(CodeGen)
add_subdirectory(FrontendAction)
add_subdirectory(Interfaces)
add_subdirectory(Lowering)
60 changes: 46 additions & 14 deletions clang/lib/CIR/FrontendAction/CIRGenAction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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");
Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

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

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");

?

Copy link
Collaborator

@AaronBallman AaronBallman Jan 30, 2025

Choose a reason for hiding this comment

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

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.

}
}

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};
Expand All @@ -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,
Copy link
Member

@lanza lanza Jan 27, 2025

Choose a reason for hiding this comment

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

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

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");
Expand All @@ -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) {
Expand All @@ -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,
Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

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 instead of using auto.

std::move(MLIRCtx), LLVMCtx);

BackendAction BEAction = getBackendActionFromOutputType(Action);
emitBackendOutput(CI, CI.getCodeGenOpts(),
C.getTargetInfo().getDataLayoutString(),
LLVMModule.get(), BEAction, FS,
std::move(OutputStream));
break;
}
}
}
};
Expand All @@ -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");
}
Expand All @@ -96,13 +126,15 @@ 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;
}

void EmitCIRAction::anchor() {}
EmitCIRAction::EmitCIRAction(mlir::MLIRContext *MLIRCtx)
: CIRGenAction(OutputType::EmitCIR, MLIRCtx) {}

void EmitLLVMAction::anchor() {}
EmitLLVMAction::EmitLLVMAction(mlir::MLIRContext *MLIRCtx)
: CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {}
1 change: 1 addition & 0 deletions clang/lib/CIR/FrontendAction/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ add_clang_library(clangCIRFrontendAction
clangAST
clangFrontend
clangCIR
clangCIRLoweringDirectToLLVM
MLIRCIR
MLIRIR
)
1 change: 1 addition & 0 deletions clang/lib/CIR/Lowering/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add_subdirectory(DirectToLLVM)
Copy link
Collaborator

@erichkeane erichkeane Jan 28, 2025

Choose a reason for hiding this comment

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

newline at end of file here too, because github.

8 changes: 8 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
set(LLVM_LINK_COMPONENTS
Core
Support
)

add_clang_library(clangCIRLoweringDirectToLLVM
LowerToLLVM.cpp
)
41 changes: 41 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

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

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.


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();
Copy link
Member

@lanza lanza Jan 27, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bcardosolopes bcardosolopes Jan 27, 2025

Choose a reason for hiding this comment

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

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

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
8 changes: 7 additions & 1 deletion clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Expand Down
8 changes: 8 additions & 0 deletions clang/test/CIR/Lowering/hello.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
Loading