Make MMIWP not have ownership over MMI + Make MMI Only Use an External MCContext #105541
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-tools-llvm-exegesis Author: Matin Raayai (matinraayai) ChangesThis PR makes the following changes, addressing issues raised in #104834 and #98770:
As expected, this change touched upon a lot of places in the LLVM monorepo; But I wanted to point out some breaking ones:
For now this PR is a WIP; The first two changes are necessary, and will have to be done sometime in the future as a part of the migration effort to the new PM; The 3rd change (only taking external MCContext) makes MMI's code cleaner, but might break code in other places where it relies on the MMIWP implicitly reseting its managed context. It might be possible to break this PR down to two parts instead if it proves to be challenging to merge. CC: @arsenm @weiweichen Patch is 72.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105541.diff 45 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 34c08818dbb9ad..7e5ebe34dc98ba 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -26,6 +26,7 @@
#include "llvm/Bitcode/BitcodeReader.h"
#include "llvm/Bitcode/BitcodeWriter.h"
#include "llvm/Bitcode/BitcodeWriterPass.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
#include "llvm/CodeGen/RegAllocRegistry.h"
#include "llvm/CodeGen/SchedulerRegistry.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
@@ -167,7 +168,8 @@ class EmitAssemblyHelper {
///
/// \return True on success.
bool AddEmitPasses(legacy::PassManager &CodeGenPasses, BackendAction Action,
- raw_pwrite_stream &OS, raw_pwrite_stream *DwoOS);
+ MachineModuleInfo &MMI, raw_pwrite_stream &OS,
+ raw_pwrite_stream *DwoOS);
std::unique_ptr<llvm::ToolOutputFile> openOutputFile(StringRef Path) {
std::error_code EC;
@@ -577,6 +579,7 @@ void EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
bool EmitAssemblyHelper::AddEmitPasses(legacy::PassManager &CodeGenPasses,
BackendAction Action,
+ MachineModuleInfo &MMI,
raw_pwrite_stream &OS,
raw_pwrite_stream *DwoOS) {
// Add LibraryInfo.
@@ -588,7 +591,7 @@ bool EmitAssemblyHelper::AddEmitPasses(legacy::PassManager &CodeGenPasses,
// this also adds codegenerator level optimization passes.
CodeGenFileType CGFT = getCodeGenFileType(Action);
- if (TM->addPassesToEmitFile(CodeGenPasses, OS, DwoOS, CGFT,
+ if (TM->addPassesToEmitFile(CodeGenPasses, OS, DwoOS, CGFT, MMI,
/*DisableVerify=*/!CodeGenOpts.VerifyModule)) {
Diags.Report(diag::err_fe_unable_to_interface_with_target);
return false;
@@ -1116,6 +1119,8 @@ void EmitAssemblyHelper::RunCodegenPipeline(
// does not work with the codegen pipeline.
// FIXME: make the new PM work with the codegen pipeline.
legacy::PassManager CodeGenPasses;
+ std::unique_ptr<llvm::MCContext> MCCtx;
+ std::unique_ptr<llvm::MachineModuleInfo> MMI;
// Append any output we need to the pass manager.
switch (Action) {
@@ -1129,7 +1134,12 @@ void EmitAssemblyHelper::RunCodegenPipeline(
if (!DwoOS)
return;
}
- if (!AddEmitPasses(CodeGenPasses, Action, *OS,
+ MCCtx = std::make_unique<MCContext>(
+ TM->getTargetTriple(), TM->getMCAsmInfo(), TM->getMCRegisterInfo(),
+ TM->getMCSubtargetInfo(), nullptr, &TM->Options.MCOptions, false);
+
+ MMI = TM->createMachineModuleInfo(*MCCtx);
+ if (!AddEmitPasses(CodeGenPasses, Action, *MMI, *OS,
DwoOS ? &DwoOS->os() : nullptr))
// FIXME: Should we handle this error differently?
return;
diff --git a/clang/lib/Interpreter/DeviceOffload.cpp b/clang/lib/Interpreter/DeviceOffload.cpp
index 07c9e3005e5fd3..29a94c372c4df8 100644
--- a/clang/lib/Interpreter/DeviceOffload.cpp
+++ b/clang/lib/Interpreter/DeviceOffload.cpp
@@ -16,8 +16,10 @@
#include "clang/CodeGen/ModuleBuilder.h"
#include "clang/Frontend/CompilerInstance.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
#include "llvm/IR/LegacyPassManager.h"
#include "llvm/IR/Module.h"
+#include "llvm/MC/MCContext.h"
#include "llvm/MC/TargetRegistry.h"
#include "llvm/Target/TargetMachine.h"
@@ -88,8 +90,13 @@ llvm::Expected<llvm::StringRef> IncrementalCUDADeviceParser::GeneratePTX() {
llvm::raw_svector_ostream dest(PTXCode);
llvm::legacy::PassManager PM;
- if (TargetMachine->addPassesToEmitFile(PM, dest, nullptr,
- llvm::CodeGenFileType::AssemblyFile)) {
+ llvm::MCContext MCCtx(
+ TargetMachine->getTargetTriple(), TargetMachine->getMCAsmInfo(),
+ TargetMachine->getMCRegisterInfo(), TargetMachine->getMCSubtargetInfo(),
+ nullptr, &TargetMachine->Options.MCOptions, false);
+ auto MMI = TargetMachine->createMachineModuleInfo(MCCtx);
+ if (TargetMachine->addPassesToEmitFile(
+ PM, dest, nullptr, llvm::CodeGenFileType::AssemblyFile, *MMI)) {
return llvm::make_error<llvm::StringError>(
"NVPTX backend cannot produce PTX code.",
llvm::inconvertibleErrorCode());
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index 9fea1fdcd5fb46..797f7b44560400 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -20,6 +20,7 @@
#include "llvm/BinaryFormat/Magic.h"
#include "llvm/Bitcode/BitcodeWriter.h"
#include "llvm/CodeGen/CommandFlags.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
#include "llvm/Frontend/Offloading/OffloadWrapper.h"
#include "llvm/Frontend/Offloading/Utility.h"
#include "llvm/IR/Constants.h"
@@ -27,6 +28,7 @@
#include "llvm/IR/Module.h"
#include "llvm/IRReader/IRReader.h"
#include "llvm/LTO/LTO.h"
+#include "llvm/MC/MCContext.h"
#include "llvm/MC/TargetRegistry.h"
#include "llvm/Object/Archive.h"
#include "llvm/Object/ArchiveWriter.h"
@@ -1051,10 +1053,14 @@ Expected<StringRef> compileModule(Module &M, OffloadKind Kind) {
auto OS = std::make_unique<llvm::raw_fd_ostream>(FD, true);
legacy::PassManager CodeGenPasses;
+ MCContext MCCtx(
+ TM->getTargetTriple(), TM->getMCAsmInfo(), TM->getMCRegisterInfo(),
+ TM->getMCSubtargetInfo(), nullptr, &TM->Options.MCOptions, false);
+ auto MMI = TM->createMachineModuleInfo(MCCtx);
TargetLibraryInfoImpl TLII(Triple(M.getTargetTriple()));
CodeGenPasses.add(new TargetLibraryInfoWrapperPass(TLII));
if (TM->addPassesToEmitFile(CodeGenPasses, *OS, nullptr,
- CodeGenFileType::ObjectFile))
+ CodeGenFileType::ObjectFile, *MMI))
return createStringError("Failed to execute host backend");
CodeGenPasses.run(M);
diff --git a/llvm/include/llvm/CodeGen/MachineModuleInfo.h b/llvm/include/llvm/CodeGen/MachineModuleInfo.h
index 310cc4b2abb772..3fb0979ec20990 100644
--- a/llvm/include/llvm/CodeGen/MachineModuleInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineModuleInfo.h
@@ -83,13 +83,11 @@ class MachineModuleInfo {
friend class MachineModuleInfoWrapperPass;
friend class MachineModuleAnalysis;
+ /// This is the TargetMachine used for the entire code generator.
const LLVMTargetMachine &TM;
/// This is the MCContext used for the entire code generator.
- MCContext Context;
- // This is an external context, that if assigned, will be used instead of the
- // internal context.
- MCContext *ExternalContext = nullptr;
+ MCContext &Context;
/// This is the LLVM Module being worked on.
const Module *TheModule = nullptr;
@@ -106,15 +104,15 @@ class MachineModuleInfo {
const Function *LastRequest = nullptr; ///< Used for shortcut/cache.
MachineFunction *LastResult = nullptr; ///< Used for shortcut/cache.
- MachineModuleInfo &operator=(MachineModuleInfo &&MMII) = delete;
-
public:
- explicit MachineModuleInfo(const LLVMTargetMachine *TM = nullptr);
+ explicit MachineModuleInfo(const LLVMTargetMachine &TM,
+ MCContext &Context);
- explicit MachineModuleInfo(const LLVMTargetMachine *TM,
- MCContext *ExtContext);
+ /// Deleted copy constructor
+ MachineModuleInfo(MachineModuleInfo &MMI) = delete;
- MachineModuleInfo(MachineModuleInfo &&MMII);
+ /// Deleted copy assignment operator
+ MachineModuleInfo &operator=(MachineModuleInfo &MMII) = delete;
~MachineModuleInfo();
@@ -124,10 +122,10 @@ class MachineModuleInfo {
const LLVMTargetMachine &getTarget() const { return TM; }
const MCContext &getContext() const {
- return ExternalContext ? *ExternalContext : Context;
+ return Context;
}
MCContext &getContext() {
- return ExternalContext ? *ExternalContext : Context;
+ return Context;
}
const Module *getModule() const { return TheModule; }
@@ -169,14 +167,12 @@ class MachineModuleInfo {
}; // End class MachineModuleInfo
class MachineModuleInfoWrapperPass : public ImmutablePass {
- MachineModuleInfo MMI;
+ MachineModuleInfo &MMI;
public:
static char ID; // Pass identification, replacement for typeid
- explicit MachineModuleInfoWrapperPass(const LLVMTargetMachine *TM = nullptr);
- explicit MachineModuleInfoWrapperPass(const LLVMTargetMachine *TM,
- MCContext *ExtContext);
+ explicit MachineModuleInfoWrapperPass(MachineModuleInfo &MMI);
// Initialization and Finalization
bool doInitialization(Module &) override;
diff --git a/llvm/include/llvm/Target/TargetMachine.h b/llvm/include/llvm/Target/TargetMachine.h
index c3e9d41315f617..2574fb80a5702d 100644
--- a/llvm/include/llvm/Target/TargetMachine.h
+++ b/llvm/include/llvm/Target/TargetMachine.h
@@ -34,7 +34,7 @@ using ModulePassManager = PassManager<Module>;
class Function;
class GlobalValue;
-class MachineModuleInfoWrapperPass;
+class MachineModuleInfo;
class Mangler;
class MCAsmInfo;
class MCContext;
@@ -375,27 +375,27 @@ class TargetMachine {
/// with the new pass manager. Only affects the "default" AAManager.
virtual void registerDefaultAliasAnalyses(AAManager &) {}
+ /// Creates a new instance of \c MachineModuleInfo to be used for code
+ /// generation for this \c TargetMachine
+ virtual std::unique_ptr<MachineModuleInfo>
+ createMachineModuleInfo(MCContext &Ctx) const = 0;
+
/// Add passes to the specified pass manager to get the specified file
- /// emitted. Typically this will involve several steps of code generation.
+ /// emitted. Typically this will involve several steps of code generation.
/// This method should return true if emission of this file type is not
/// supported, or false on success.
- /// \p MMIWP is an optional parameter that, if set to non-nullptr,
- /// will be used to set the MachineModuloInfo for this PM.
- virtual bool
- addPassesToEmitFile(PassManagerBase &, raw_pwrite_stream &,
- raw_pwrite_stream *, CodeGenFileType,
- bool /*DisableVerify*/ = true,
- MachineModuleInfoWrapperPass *MMIWP = nullptr) {
+ /// \p MMI must be created externally before being passed to this function
+ virtual bool addPassesToEmitFile(PassManagerBase &, raw_pwrite_stream &,
+ raw_pwrite_stream *, CodeGenFileType,
+ MachineModuleInfo &MMI,
+ bool /*DisableVerify*/ = true) {
return true;
}
/// Add passes to the specified pass manager to get machine code emitted with
- /// the MCJIT. This method returns true if machine code is not supported. It
- /// fills the MCContext Ctx pointer which can be used to build custom
- /// MCStreamer.
- ///
- virtual bool addPassesToEmitMC(PassManagerBase &, MCContext *&,
- raw_pwrite_stream &,
+ /// the MCJIT. This method returns true if machine code is not supported.
+ virtual bool addPassesToEmitMC(PassManagerBase &, raw_pwrite_stream &,
+ MachineModuleInfo &,
bool /*DisableVerify*/ = true) {
return true;
}
@@ -459,15 +459,18 @@ class LLVMTargetMachine : public TargetMachine {
/// for generating a pipeline of CodeGen passes.
virtual TargetPassConfig *createPassConfig(PassManagerBase &PM);
+ /// Creates a new instance of \c MachineModuleInfo to be used for code
+ /// generation for this \c TargetMachine
+ virtual std::unique_ptr<MachineModuleInfo>
+ createMachineModuleInfo(MCContext &Ctx) const override;
+
/// Add passes to the specified pass manager to get the specified file
- /// emitted. Typically this will involve several steps of code generation.
- /// \p MMIWP is an optional parameter that, if set to non-nullptr,
- /// will be used to set the MachineModuloInfo for this PM.
- bool
- addPassesToEmitFile(PassManagerBase &PM, raw_pwrite_stream &Out,
- raw_pwrite_stream *DwoOut, CodeGenFileType FileType,
- bool DisableVerify = true,
- MachineModuleInfoWrapperPass *MMIWP = nullptr) override;
+ /// emitted. Typically this will involve several steps of code generation.
+ /// \p MMI must be created externally before being passed to this function
+ bool addPassesToEmitFile(PassManagerBase &PM, raw_pwrite_stream &Out,
+ raw_pwrite_stream *DwoOut, CodeGenFileType FileType,
+ MachineModuleInfo &MMI,
+ bool DisableVerify = true) override;
virtual Error buildCodeGenPipeline(ModulePassManager &, raw_pwrite_stream &,
raw_pwrite_stream *, CodeGenFileType,
@@ -478,11 +481,9 @@ class LLVMTargetMachine : public TargetMachine {
}
/// Add passes to the specified pass manager to get machine code emitted with
- /// the MCJIT. This method returns true if machine code is not supported. It
- /// fills the MCContext Ctx pointer which can be used to build custom
- /// MCStreamer.
- bool addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
- raw_pwrite_stream &Out,
+ /// the MCJIT. This method returns true if machine code is not supported.
+ bool addPassesToEmitMC(PassManagerBase &PM, raw_pwrite_stream &Out,
+ MachineModuleInfo &MMI,
bool DisableVerify = true) override;
/// Returns true if the target is expected to pass all machine verifier
diff --git a/llvm/lib/CodeGen/LLVMTargetMachine.cpp b/llvm/lib/CodeGen/LLVMTargetMachine.cpp
index d0dfafeaef561f..1236c05328fff9 100644
--- a/llvm/lib/CodeGen/LLVMTargetMachine.cpp
+++ b/llvm/lib/CodeGen/LLVMTargetMachine.cpp
@@ -206,13 +206,16 @@ Expected<std::unique_ptr<MCStreamer>> LLVMTargetMachine::createMCStreamer(
return std::move(AsmStreamer);
}
+std::unique_ptr<MachineModuleInfo>
+LLVMTargetMachine::createMachineModuleInfo(MCContext &Ctx) const {
+ return std::make_unique<MachineModuleInfo>(*this, Ctx);
+}
+
bool LLVMTargetMachine::addPassesToEmitFile(
PassManagerBase &PM, raw_pwrite_stream &Out, raw_pwrite_stream *DwoOut,
- CodeGenFileType FileType, bool DisableVerify,
- MachineModuleInfoWrapperPass *MMIWP) {
+ CodeGenFileType FileType, MachineModuleInfo &MMI, bool DisableVerify) {
// Add common CodeGen passes.
- if (!MMIWP)
- MMIWP = new MachineModuleInfoWrapperPass(this);
+ auto *MMIWP = new MachineModuleInfoWrapperPass(MMI);
TargetPassConfig *PassConfig =
addPassesToGenerateCode(*this, PM, DisableVerify, *MMIWP);
if (!PassConfig)
@@ -233,14 +236,14 @@ bool LLVMTargetMachine::addPassesToEmitFile(
/// addPassesToEmitMC - Add passes to the specified pass manager to get
/// machine code emitted with the MCJIT. This method returns true if machine
-/// code is not supported. It fills the MCContext Ctx pointer which can be
-/// used to build custom MCStreamer.
-///
-bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
+/// code is not supported. It uses the MCContext Ctx of \p MMI so that it
+/// can be used to build custom MCStreamer.
+bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM,
raw_pwrite_stream &Out,
+ MachineModuleInfo &MMI,
bool DisableVerify) {
// Add common CodeGen passes.
- MachineModuleInfoWrapperPass *MMIWP = new MachineModuleInfoWrapperPass(this);
+ MachineModuleInfoWrapperPass *MMIWP = new MachineModuleInfoWrapperPass(MMI);
TargetPassConfig *PassConfig =
addPassesToGenerateCode(*this, PM, DisableVerify, *MMIWP);
if (!PassConfig)
@@ -248,7 +251,7 @@ bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
assert(TargetPassConfig::willCompleteCodeGenPipeline() &&
"Cannot emit MC with limited codegen pipeline");
- Ctx = &MMIWP->getMMI().getContext();
+ auto &Ctx = MMI.getContext();
// libunwind is unable to load compact unwind dynamically, so we must generate
// DWARF unwind info for the JIT.
Options.MCOptions.EmitDwarfUnwind = EmitDwarfUnwindType::Always;
@@ -258,7 +261,7 @@ bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
const MCSubtargetInfo &STI = *getMCSubtargetInfo();
const MCRegisterInfo &MRI = *getMCRegisterInfo();
std::unique_ptr<MCCodeEmitter> MCE(
- getTarget().createMCCodeEmitter(*getMCInstrInfo(), *Ctx));
+ getTarget().createMCCodeEmitter(*getMCInstrInfo(), Ctx));
MCAsmBackend *MAB =
getTarget().createMCAsmBackend(STI, MRI, Options.MCOptions);
if (!MCE || !MAB)
@@ -266,7 +269,7 @@ bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
const Triple &T = getTargetTriple();
std::unique_ptr<MCStreamer> AsmStreamer(getTarget().createMCObjectStreamer(
- T, *Ctx, std::unique_ptr<MCAsmBackend>(MAB), MAB->createObjectWriter(Out),
+ T, Ctx, std::unique_ptr<MCAsmBackend>(MAB), MAB->createObjectWriter(Out),
std::move(MCE), STI));
// Create the AsmPrinter, which takes ownership of AsmStreamer if successful.
diff --git a/llvm/lib/CodeGen/MachineModuleInfo.cpp b/llvm/lib/CodeGen/MachineModuleInfo.cpp
index c66495969b4e67..55ef4916cd0101 100644
--- a/llvm/lib/CodeGen/MachineModuleInfo.cpp
+++ b/llvm/lib/CodeGen/MachineModuleInfo.cpp
@@ -30,39 +30,14 @@ void MachineModuleInfo::initialize() {
}
void MachineModuleInfo::finalize() {
- Context.reset();
- // We don't clear the ExternalContext.
-
delete ObjFileMMI;
ObjFileMMI = nullptr;
}
-MachineModuleInfo::MachineModuleInfo(MachineModuleInfo &&MMI)
- : TM(std::move(MMI.TM)),
- Context(TM.getTargetTriple(), TM.getMCAsmInfo(), TM.getMCRegisterInfo(),
- TM.getMCSubtargetInfo(), nullptr, &TM.Options.MCOptions, false),
- MachineFunctions(std::move(MMI.MachineFunctions)) {
+MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine &TM,
+ MCContext &Context)
+ : TM(TM), Context(Context) {
Context.setObjectFileInfo(TM.getObjFileLowering());
- ObjFileMMI = MMI.ObjFileMMI;
- ExternalContext = MMI.ExternalContext;
- TheModule = MMI.TheModule;
-}
-
-MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine *TM)
- : TM(*TM), Context(TM->getTargetTriple(), TM->getMCAsmInfo(),
- TM->getMCRegisterInfo(), TM->getMCSubtargetInfo(),
- nullptr, &TM->Options.MCOptions, false) {
- Context.setObjectFileInfo(TM->getObjFileLowering());
- initialize();
-}
-
-MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine *TM,
- MCContext *ExtContext)
- : TM(*TM), Context(TM->getTargetTriple(), TM->getMCAsmInfo(),
- TM->getMCRegisterInfo(), TM->getMCSubtargetInfo(),
- nullptr, &TM->Options.MCOptions, false),
- ExternalContext(ExtContext) {
- Context.setObjectFileInfo(TM->getObjFileLowering());
initialize();
}
@@ -137,9 +112,7 @@ class FreeMachineFunction : public FunctionPass {
return true;
}
- StringRef getPassName() const override {
- return "Free MachineFunction";
- }
+ StringRef getPassName() const override { return "Free MachineFunction"; }
};
} // end anonymous namespace
@@ -151,14 +124,8 @@ FunctionPass *llvm::createFreeMachineFunctionPass() {
}
MachineModuleInfoWrapperPass::MachineModuleInfoWrapperPass(
- const LLVMTargetMachine *TM)
- : ImmutablePass(ID), MMI(TM) {
- initializeMachineModuleInfoWrapperPassPass(*PassRegistry::getPassRegistry());
-}
-
-MachineModuleInfoWrapperPass::MachineModuleInfoWrapperPass(
- const LLVMTargetMachine *TM, MCContext *ExtContext)
- : ImmutablePass(ID), MMI(TM, ExtContext) {
+ MachineModuleInfo &MMI)
+ : ImmutablePass(ID), MMI(MMI) {
initializeMachineModuleInfoWrapperPassPass(*PassRegistry::getPassRegistry());
}
diff --git a/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp b/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp
index 0d7a51bfe73753..fc9f09c3d3064c 100644
--- a/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp
+++ b/llvm/lib/ExecutionEngine/MCJIT/...
[truncated]
|
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
ff58a94 to
e24b3f6
Compare
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
8c2cf2b to
ad37447
Compare
llvmbot
added
mlir:llvm
flang:driver
mlir
flang
I don't quite follow what the issue is here, but I'm off until Tuesday so I'll look again next week (I hope) |
It's just an abstraction issue; I think it becomes more clear once you read the diffs. |
|
I'm just back from vacation, taking a look looks like this has merge conflicts could you simplify the commit title (maybe move some details into the description) |
matinraayai
changed the title
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
ad37447 to
0a7c35d
Compare
…migrated to the new TargetMachine API.
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
0a7c35d to
5227576
Compare
|
@aeubanks should be ready for review again. |
|
sorry for the delay after looking at MMI/MCContext, I agree that MMI shouldn't own MCContext, but do we even need a reference from MMI to MCContext? they are different layers of codegen IIUC. if it's possible to completely separate them we should do that (please correct me if this doesn't make sense since I haven't spent too much time in codegen) |
@aeubanks It's not impossible to separate them completely. In theory, you can just pass the context to the wrapper pass instead. @arsenm any thoughts on this? |
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
6ec9fce to
b088431
Compare
matinraayai
force-pushed
the
fix-mmi-context-ownership
branch
from
b088431 to
68c3fe6
Compare
The MachineModuleInfo is the container for all the MachineFunctions (which do hold a reference to the MCContext), so it kind of makes sense to keep it there. But it does look like it should be simple to remove the reference here. So I would say it's better to just remove it |
|
In that case, I will modify this PR so that:
I'll report back in case I run into unexpected issues. |
|
@aeubanks @arsenm after looking into this in more detail, I realized that the Hence I don't think it's a good idea to remove the |
The AsmPrinter is just an ordinary ModulePass. The initialization can just set a MMI member? |
I agree that separating MachineFunction &MachineModuleInfo::getOrCreateMachineFunction(Function &F, MCContext &MCCtx) {
// Shortcut for the common case where a sequence of MachineFunctionPasses
// all query for the same Function.
if (LastRequest == &F)
return *LastResult;
auto I = MachineFunctions.insert(
std::make_pair(&F, std::unique_ptr<MachineFunction>()));
MachineFunction *MF;
if (I.second) {
// No pre-existing machine function, create a new one.
const TargetSubtargetInfo &STI = *TM.getSubtargetImpl(F);
MF = new MachineFunction(F, TM, STI, MCCtx, NextFnNum++);
MF->initTargetMachineFunctionInfo(STI);
// MRI callback for target specific initializations.
TM.registerMachineRegisterInfoCallback(*MF);
// Update the set entry.
I.first->second.reset(MF);
} else {
MF = I.first->second.get();
}
LastRequest = &F;
LastResult = MF;
return *MF;
}Also the constructor for MMI sets the context's object file info here, which I think has a similar issue: MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine &TM,
MCContext &Context)
: TM(TM), Context(Context) {
Context.setObjectFileInfo(TM.getObjFileLowering());
initialize();
}Again, for both these cases, it's possible to remove the |
|
hmm yeah it does seem that is it possible to split out the "MMIWP doesn't own MMI" change? that seems less controversial and easier to review/merge and makes the |
|
@aeubanks sure I can split the PR into two parts:
I will make a new PR for each of them and close this one once both are merged. |
This PR makes the following changes, addressing issues raised in #104834 and #98770:
Module's where it also take a reference to the externally createdLLVMContextit uses.As expected, this change touched upon a lot of places in the LLVM monorepo; But I wanted to point out some breaking ones:
TargetMachineinterface functionsaddPassesToEmitFileandaddPassesToEmitMCnow require a reference to an MMI; This IMO breaks the abstraction of theTargetMachine, since anMMIrequires aLLVMTargetMachine, and if you have aTargetMachineyou should do the dreaded casting toLLVMTargetMachinein order to create it. Now I created a factory method for MMIs in theTargetMachineinterface to remedy this issue, but I still don't like it since again, it is only implemented forLLVMTargetMachine.TargetMachineinterface.For now this PR is a WIP; The first two changes are necessary, and will have to be done sometime in the future as a part of the migration effort to the new PM; The 3rd change (only taking external MCContext) makes MMI's code cleaner, but might break code in other places where it relies on the MMIWP implicitly reseting its managed context. It might be possible to break this PR down to two parts instead if it proves to be challenging to merge.
CC: @arsenm @weiweichen