Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 4 additions & 1 deletion clang/include/clang/CIR/CIRGenerator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

std::unique_ptr<clang::CIRGen::CIRGenModule> cgm;

public:
Expand All @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a const member function?

return mlirContext;
};
};

} // namespace cir
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/CIR/FrontendAction/CIRGenAction.h
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
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
1 change: 1 addition & 0 deletions clang/lib/CIR/CMakeLists.txt
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)
2 changes: 1 addition & 1 deletion clang/lib/CIR/CodeGen/CIRGenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ void CIRGenerator::Initialize(ASTContext &astContext) {

this->astContext = &astContext;

mlirContext = std::make_unique<mlir::MLIRContext>();
mlirContext = std::make_shared<mlir::MLIRContext>();
mlirContext->loadDialect<cir::CIRDialect>();
cgm = std::make_unique<clang::CIRGen::CIRGenModule>(
*mlirContext.get(), astContext, codeGenOpts, diags);
Expand Down
66 changes: 48 additions & 18 deletions clang/lib/CIR/FrontendAction/CIRGenAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,56 @@
//===----------------------------------------------------------------------===//

#include "clang/CIR/FrontendAction/CIRGenAction.h"
#include "clang/CIR/CIRGenerator.h"
#include "clang/Frontend/CompilerInstance.h"

#include "mlir/IR/MLIRContext.h"
#include "mlir/IR/OwningOpRef.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"

using namespace cir;
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

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

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

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

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

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::shared_ptr<mlir::MLIRContext> MLIRCtx, llvm::LLVMContext &LLVMCtx) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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};
IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
std::unique_ptr<CIRGenerator> Gen;

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,
CIRGenConsumer(CIRGenAction::OutputType Action, 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");
Expand All @@ -58,6 +72,7 @@ class CIRGenConsumer : public clang::ASTConsumer {
void HandleTranslationUnit(ASTContext &C) override {
Gen->HandleTranslationUnit(C);
mlir::ModuleOp MlirModule = Gen->getModule();
std::shared_ptr<mlir::MLIRContext> MLIRCtx = Gen->getContext();
switch (Action) {
case CIRGenAction::OutputType::EmitCIR:
if (OutputStream && MlirModule) {
Expand All @@ -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,
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 instead of using auto.

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 +110,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 @@ -95,14 +123,16 @@ CIRGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
if (!Out)
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));
auto Result =
std::make_unique<cir::CIRGenConsumer>(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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add_subdirectory(DirectToLLVM)
8 changes: 8 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt
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
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

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

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 MOp, LLVMContext &LLVMCtx) {
llvm::TimeTraceScope scope("lower from CIR to LLVM directly");

std::optional<StringRef> ModuleName = MOp.getName();
auto M = std::make_unique<llvm::Module>(
ModuleName ? *ModuleName : "CIRToLLVMModule", LLVMCtx);

if (!M)
report_fatal_error("Lowering from LLVMIR dialect to llvm IR failed!");
Comment on lines +33 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return M;
}
} // namespace direct
} // namespace cir
8 changes: 7 additions & 1 deletion clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
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
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