Make Ownership of MachineModuleInfo in Its Wrapper Pass External #110443

matinraayai · 2024-09-29T23:45:35Z

Following up on discussions in #105541, this PR is the first part of #105541, which removes ownership of the llvm::MachineModuleInfoWrapperPass over its encapsulated llvm::MachineModuleInfo, allowing better control over the MMI's lifetime.

After this PR is merged, I plan to:

Move the MC emission functions in TargetMachine to LLVMTargetMachine. With the changes in this PR, we explicitly assume in both addPassesToEmitFile and addPassesToEmitMC that the TargetMachine is an LLVMTargetMachine; Hence it does not make sense for these functions to be present in the TargetMachine interface.
Make MMI only take external context, which originally was the second part of Make MMIWP not have ownership over MMI + Make MMI Only Use an External MCContext #105541.

cc @aeubanks @arsenm

…Machine.

… API.

…ssion API.

…on API.

…API.

…ges to a minimum.

…ssion API.

…n API.

github-actions · 2024-09-29T23:47:23Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

github-actions · 2024-09-29T23:49:07Z

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

arsenm · 2024-09-30T16:26:25Z

Move the MC emission functions in TargetMachine to LLVMTargetMachine. With the changes in this PR, we explicitly assume in both addPassesToEmitFile and addPassesToEmitMC that the TargetMachine is an LLVMTargetMachine; Hence it does not make sense for these functions to be present in the TargetMachine interface.

Was this already implicitly assumed? IIRC there was some layering reason why this is the way it was. There were previous attempts to merge these before, which were abandoned:

https://lists.llvm.org/pipermail/llvm-dev/2017-October/117907.html

https://reviews.llvm.org/D38482
https://reviews.llvm.org/D38489

matinraayai · 2024-09-30T16:33:47Z

Move the MC emission functions in TargetMachine to LLVMTargetMachine. With the changes in this PR, we explicitly assume in both addPassesToEmitFile and addPassesToEmitMC that the TargetMachine is an LLVMTargetMachine; Hence it does not make sense for these functions to be present in the TargetMachine interface.

Was this already implicitly assumed? IIRC there was some layering reason why this is the way it was. There were previous attempts to merge these before, which were abandoned:

https://lists.llvm.org/pipermail/llvm-dev/2017-October/117907.html

https://reviews.llvm.org/D38482 https://reviews.llvm.org/D38489

@arsenm Initially I was going to move the MC file emission functions to LLVMTargetMachine but I decided against it, as it would make the PR harder to review.

I don't intend on merging the two interfaces together; I only think the MC file emission functions addPassesToEmitFile and addPassesToEmitMC need to be moved to LLVMTargetMachine. This is because creating an MMI requires the TargetMachine to be an LLVMTargetMachine in the first place. Other functions in TargetMachine that are not MC-related should remain the way it was, which I think should respect the original layering.

…ings in the C-API + linked any libraries that depends on MC file emission to CodeGen.

matinraayai · 2024-10-01T20:36:03Z

@MatzeB @arsenm in the latest commit I had to fix the issue you ran into in the following links @arsenm mentioned earlier:
https://lists.llvm.org/pipermail/llvm-dev/2017-October/117907.html

https://reviews.llvm.org/D38482
https://reviews.llvm.org/D38489

I can live with TargetMachine and LLVMTargetMachine being separate entities, with TargetMachine being the "frontend" and LLVMTargetMachine being any concrete subclass with codegen support as @MatzeB explained in the past.

With this patch, if any library requires MC code emission of any kind, it has to explicitly link to libCodeGen, since it has to be able to access the MMI constructor/destructor. I still believe that addPassesToEmitMC and addPassesToEmitFile need to be moved to LLVMTargetMachine to further enforce the distinction between LLVMTargetMachine and TargetMachine, as well as any other interface that explicitly uses CodeGen or MC primitives (e.g. MachineFunctionInfoYaml things).

llvm/lib/CodeGen/LLVMTargetMachineC.cpp

aeubanks

I'm still trying to wrap my head around LLVMTargetMachine vs TargetMachine, let me do some reading

aeubanks · 2024-10-03T19:50:05Z

clang/lib/CodeGen/BackendUtil.cpp

@@ -1162,6 +1165,7 @@ 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<MachineModuleInfo> MMI;


can this just be MachineModuleInfo MMI; below?

I'll change it to a normal construction with the TM.

aeubanks · 2024-10-03T19:53:40Z

llvm/include/llvm/CodeGen/MachineModuleInfo.h

@@ -106,16 +106,18 @@ class MachineModuleInfo {
  const Function *LastRequest = nullptr; ///< Used for shortcut/cache.
  MachineFunction *LastResult = nullptr; ///< Used for shortcut/cache.

-  MachineModuleInfo &operator=(MachineModuleInfo &&MMII) = delete;
+  /// Deleted copy constructor


comment is not useful, I'd either say why it's deleted or just remove the comment

aeubanks

ah there's more context in https://reviews.llvm.org/D123964

aeubanks

I see that MMI really is a Codegen concept and not a Target concept, so that's why it takes an LLVMTargetMachine instead of TargetMachine. However, the static_casts everywhere are extremely unfortunate. And it doesn't make sense to make the return type of Target::createTargetMachine() LLVMTargetMachine instead of TargetMachine. Perhaps alternatively we make the MMI constructor take TargetMachine and cast it inside the constructor to LLVMTargetMachine, so we only pay the cost of this weird distinction in one place rather than everywhere? wdyt?

matinraayai · 2024-10-04T15:22:21Z

I dug up the commit that introduced LLVMTargetMachine: 12e9730. It dates back to before when the MC layer was created. It seems the motivation was to allow a hypothetical target to generate code using whatever code generator it wants internally (be it the one provided by LLVM at the time or some other external library). The MC stuff was later on added on top of it around 2010, which I think made this abstraction a bit pointless, since MC and CodeGen integrate tightly together, and there's no point to have support for only one layer.

Given the issues faced when joining TM and LLVMTM, I think this abstraction should be respected. The key takeaway from this abstraction is that TM must have a function that generates object files/MC; It's just that those interface functions should be void of any CodeGen related constructs (even the MMIWP). TLDR: We should follow this rule of thumb: If it uses LLVM's CodeGen it belongs to the LLVMTM class, otherwise it belongs to TM (The same goes for all the MachineFunctionInfo stuff; They should be moved to LLVMTM).

This is a cause of concern for managing the lifetime of MMI: For TM interfaces, MMI's lifetime should be managed by the MMIWP pass, otherwise it will get deleted when it goes out of the scope of the pass building function. For LLVMTM interface, however, MMI should be managed externally by the interface user. I think both should exist.

This also relates to a question that I had regarding the new PM codegen interface buildCodeGenPipeline @aeubanks: how exactly is the MMI's lifetime managed after calling this function? If it's possible I want to talk more about it offline (You can find me on LLVM's Discord).

I see that MMI really is a Codegen concept and not a Target concept, so that's why it takes an LLVMTargetMachine instead of TargetMachine. However, the static_casts everywhere are extremely unfortunate. And it doesn't make sense to make the return type of Target::createTargetMachine() LLVMTargetMachine instead of TargetMachine. Perhaps alternatively we make the MMI constructor take TargetMachine and cast it inside the constructor to LLVMTargetMachine, so we only pay the cost of this weird distinction in one place rather than everywhere? wdyt?

To get back to your question @aeubanks I don't think we should force any casts in the MMI constructor; Instead we should address the TM/LLVMTM abstraction issue. The casting will then take care of itself. Also there should be a Target::createLLVMTargetMachine() for those who want to explicitly manage MMI's lifetime and want to use LLVm CodeGen.

@arsenm

Following discussions in #110443, and the following earlier discussions in https://lists.llvm.org/pipermail/llvm-dev/2017-October/117907.html, https://reviews.llvm.org/D38482, https://reviews.llvm.org/D38489, this PR attempts to overhaul the `TargetMachine` and `LLVMTargetMachine` interface classes. More specifically: 1. Makes `TargetMachine` the only class implemented under `TargetMachine.h` in the `Target` library. 2. `TargetMachine` contains target-specific interface functions that relate to IR/CodeGen/MC constructs, whereas before (at least on paper) it was supposed to have only IR/MC constructs. Any Target that doesn't want to use the independent code generator simply does not implement them, and returns either `false` or `nullptr`. 3. Renames `LLVMTargetMachine` to `CodeGenCommonTMImpl`. This renaming aims to make the purpose of `LLVMTargetMachine` clearer. Its interface was moved under the CodeGen library, to further emphasis its usage in Targets that use CodeGen directly. 4. Makes `TargetMachine` the only interface used across LLVM and its projects. With these changes, `CodeGenCommonTMImpl` is simply a set of shared function implementations of `TargetMachine`, and CodeGen users don't need to static cast to `LLVMTargetMachine` every time they need a CodeGen-specific feature of the `TargetMachine`. 5. More importantly, does not change any requirements regarding library linking. cc @arsenm @aeubanks

matinraayai added 14 commits September 29, 2024 16:54

Made MMIWP not have control over the lifetime of MMI.

6a78a68

Moved the MC pass creation functions from TargetMachine to LLVMTarget…

c2d984a

…Machine.

Migrated BackendUtil.cpp to the new LLVMTargetMachine code emission API.

ed40387

Migrated DeviceOffload.cpp to the new LLVMTargetMachine code emission…

75dd38a

… API.

Migrated Wasm.cpp to the new LLVMTargetMachine code emission API.

00be9bf

Migrated ClangLinkerWrapper.cpp to the new LLVMTargetMachine code emi…

7bee39f

…ssion API.

Migrated FrontendActions.cpp to the new LLVMTargetMachine code emissi…

d9fcf48

…on API.

Migrated MCJIT.cpp to the new LLVMTargetMachine code emission API.

631cbb2

Migrated CompileUtils.cpp to the new LLVMTargetMachine code emission …

e8e81db

…API.

Moved the MC emission APIs to TargetMachine again to keep the PR chan…

4cfc73e

…ges to a minimum.

Migrated LTOBackend.cpp to use the new TargetMachine MC emission API.

4ca0afb

Migrated ThinLTOCodeGenerator.cpp to use the new TargetMachine MC emi…

b33936c

…ssion API.

Migrated NVPTXTargetMachine.h to use the new TargetMachine MC emissio…

e945366

…n API.

Migrated the remaining test files to the new MMIWP constructor.

737685e

matinraayai added 3 commits September 29, 2024 19:54

clang tidy.

0a6575c

Fixed compilation issue.

753d99a

Fixed default construction of MMI in the AsmPrinterDwarfTest.

9ba7913

arsenm added the llvm:codegen label Sep 30, 2024

arsenm requested review from MatzeB, echristo and MaskRay September 30, 2024 16:07

Create LLVMTargetMachineC.cpp to implement the MC-emission related th…

ead138a

…ings in the C-API + linked any libraries that depends on MC file emission to CodeGen.

arsenm reviewed Oct 2, 2024

View reviewed changes

llvm/lib/CodeGen/LLVMTargetMachineC.cpp Outdated Show resolved Hide resolved

Added missing line at the end of LLVMTargetMachineC.cpp.

928014d

matinraayai force-pushed the mmiwp-mmi-ownership branch from 68428e3 to 928014d Compare October 2, 2024 16:04

matinraayai added 2 commits October 2, 2024 12:05

Added missing line at the end of LLVMTargetMachineC.cpp.

1beda2a

Next line issue in LLVMTargetMachineC.cpp.

d4a5620

aeubanks reviewed Oct 3, 2024

View reviewed changes

matinraayai mentioned this pull request Oct 5, 2024

Overhaul the TargetMachine and LLVMTargetMachine Classes #111234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make Ownership of MachineModuleInfo in Its Wrapper Pass External #110443

Make Ownership of MachineModuleInfo in Its Wrapper Pass External #110443

Uh oh!

matinraayai commented Sep 29, 2024

Uh oh!

github-actions bot commented Sep 29, 2024

Uh oh!

github-actions bot commented Sep 29, 2024 •

edited

Loading

Uh oh!

arsenm commented Sep 30, 2024

Uh oh!

matinraayai commented Sep 30, 2024 •

edited

Loading

Uh oh!

matinraayai commented Oct 1, 2024

Uh oh!

Uh oh!

aeubanks left a comment

Uh oh!

aeubanks Oct 3, 2024

Uh oh!

matinraayai Oct 4, 2024

Uh oh!

aeubanks Oct 3, 2024

Uh oh!

aeubanks left a comment

Uh oh!

aeubanks left a comment

Uh oh!

matinraayai commented Oct 4, 2024

Uh oh!

Uh oh!

Make Ownership of MachineModuleInfo in Its Wrapper Pass External #110443

Are you sure you want to change the base?

Make Ownership of MachineModuleInfo in Its Wrapper Pass External #110443

Uh oh!

Conversation

matinraayai commented Sep 29, 2024

Uh oh!

github-actions bot commented Sep 29, 2024

Uh oh!

github-actions bot commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm commented Sep 30, 2024

Uh oh!

matinraayai commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matinraayai commented Oct 1, 2024

Uh oh!

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

aeubanks Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

matinraayai Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

aeubanks Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

matinraayai commented Oct 4, 2024

Uh oh!

Uh oh!

github-actions bot commented Sep 29, 2024 •

edited

Loading

matinraayai commented Sep 30, 2024 •

edited

Loading