A MMIWP Constructor Initialized with the move constructor of MMI

[matinraayai]

This PR exposes the move constructor of MachineModuleInfo as a way to initialize the MachineModuleInfoWrapperPasss.

This is useful for running passes over an externally initialized MMI, and then moving it back to its original storage for further processing/inspection, as shown in the following example:

    llvm::legacy::PassManager PM;
    auto MMI = std::make_unique<llvm::MachineModuleInfo>(TM);
    ... (Modify/inspect MMI)
    /// Initialize a MMIWP using the external MMI
    auto * MMIWP = new llvm::MachineModuleInfoWrapperPass(std::move(*MMI));
    PM.add(MMIWP);
    ... (Add more passes)
    PM.run(M);
    /// Move the MMI back to its original place prevent it from being destroyed after PM goes out of scope
    MMI = std::make_unique<llvm::MachineModuleInfo>(std::move(MMIWP->getMMI()));
    .. (Inspect the MMI)

[github-actions]

⚠️ 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]

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

[arsenm]

What do you need to set in MMI?

[matinraayai]

What do you need to set in MMI?

In a nutshell, the mapping between llvm::Function and the llvm::machineFunctions.
In my code, I either:

1. I have an MMI pre-populated with MFs and MIs, much like llvm-exegesis. I put the Module and the MMI through a minimal codegen pipeline to print the assembly.
2. I have an LLVM IR Module which I put through IR and codegen pipelines right before assembly printing.

In either situations, I want to store the MMI and inspect it before/after code generation outside of a pass manager infrastructure; Hence I need control over the lifetime of the MMI so that it doesn't get destroyed by the pass manager's destructor.

Right now, I have a data structure that stores a Module as well as its MMIWP. The problem is, after running passes on the MMIWP I can't update it to store the modified MIR, because MMIWP doesn't have an assignment/move constructor.

Having this constructor removes the need to store the entire MMIWP, and allows me to store MMI before and after it is modified by the pass manager.

@arsenm I can show you the code offline in case you need more details.

[matinraayai]

@arsenm On further inspection, I think the move constructor of MMI is not correct and should be removed all together:

llvm-project/llvm/lib/CodeGen/MachineModuleInfo.cpp

Lines 57 to 67 in 8ba9ed6

	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)) {
	Context.setObjectFileInfo(TM.getObjFileLowering());
	ObjFileMMI = MMI.ObjFileMMI;
	CurCallSite = MMI.CurCallSite;
	ExternalContext = MMI.ExternalContext;
	TheModule = MMI.TheModule;
	}

Yes, the machine function map gets moved; but each machine function holds an internal reference to the original MMI and its context, and moving them makes those references stale.

At this point I think the only way to prevent an MMI from going out of scope is hold on to an MMIWP and the PMs that control its lifetime.

I could in theory refactor MMIWP to either:

1. Only support an external MMI (same as MachineModuleAnalysis), which requires a good amount of refactoring work.
2. Support both internal and external MMI (see MMI's MCContext itself)

Given that the CodeGen pipeline is being ported to the new pass manager, and the MachineModuleAnalysis only supports an external MMI, I'm not sure if this contribution will be accepted upstream.

[arsenm]

Yes, the machine function map gets moved; but each machine function holds an internal reference to the original MMI and its context, and moving them makes those references stale.

Can we remove this reference from MachineFunction? Intuitively it shouldn't be necessary. A quick grep shows limited uses, and in contexts where it should be directly queryable from the PM

[arsenm]

Yes, the machine function map gets moved; but each machine function holds an internal reference to the original MMI and its context, and moving them makes those references stale.

Can we remove this reference from MachineFunction? Intuitively it shouldn't be necessary. A quick grep shows limited uses, and in contexts where it should be directly queryable from the PM

#99652 starts moving towards this

[matinraayai]

@arsenm thanks for making a PR for removing the MMI reference; Let me know when it is merged so that I can test it with this PR.

However, I think there might be more issues with this than just a set of stale references. While trying the code example I put here in my tool code, I noticed that the MF states get corrupted once the PM operating on its original MMI goes out of scope. For this PR to merge I think I need to clarify whether an MMI should be moved at all. If we treat MMI as a "Module but for MIR" then probably not. If we treat it simply just as an MIR analysis data structure, then a move operator might be very useful.

If an MMI can be moved, then the constructor needs to be tested. We can do this by forcing the old MMI/PM out of scope and then checking its internal content. That's how I noticed the stale reference issues in the first place.

On the other hand, if an MMI cannot be moved, then it might be best if MMIWP doesn't own its MMI. I think MMIWP should also behave like the new PM MachineModuleAnalysis, where the MMI must be externally supplied to the pass.

I'm happy to move this PR in either direction, given I understand which direction is deemed appropriate by the backend maintainers.

[arsenm]

@arsenm thanks for making a PR for removing the MMI reference; Let me know when it is merged so that I can test it with this PR.

That one is only the low hanging fruit, more work is needed to actually remove the reference

[arsenm]

Will need a new PM version at some point

[matinraayai]

Closing this PR in favor of #105541.