Page MenuHomePhabricator

[LTO] Update splitCodeGen to take a reference to the module. (NFC)
ClosedPublic

Authored by fhahn on Jan 22 2021, 5:04 AM.

Details

Summary

splitCodeGen does not need to take ownership of the module, as it
currently clones the original module for each split operation.

There is an ~4 year old fixme to change that, but until this is
addressed, the function can just take a reference to the module.

This makes the transition of LTOCodeGenerator to use LTOBackend a bit
easier, because under some circumstances, LTOCodeGenerator needs to
write the original module back after codegen.

Diff Detail

Event Timeline

fhahn created this revision.Jan 22 2021, 5:04 AM
fhahn requested review of this revision.Jan 22 2021, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 5:04 AM

Where/how is the returned module used in that case? How do you avoid having it accessed later in the parallel code gen case?

Where/how is the returned module used in that case? How do you avoid having it accessed later in the parallel code gen case?

We should be able to use the returned module in the ParallelismLevel == 1 case, where we just pass a reference of the module to codegen. I think this behavior is needed for D94487 (llvm/lib/LTO/LTOCodeGenerator.cpp around line 608). This is a bit fragile/messy unfortunately, but I think we need it to preserve the existing libLTO behavior. It is also similar to the behavior of the previously used splitCodeGen https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/ParallelCG.cpp#L49. But perhaps there's a better way to factor this out.

Where/how is the returned module used in that case? How do you avoid having it accessed later in the parallel code gen case?

We should be able to use the returned module in the ParallelismLevel == 1 case, where we just pass a reference of the module to codegen. I think this behavior is needed for D94487 (llvm/lib/LTO/LTOCodeGenerator.cpp around line 608). This is a bit fragile/messy unfortunately, but I think we need it to preserve the existing libLTO behavior. It is also similar to the behavior of the previously used splitCodeGen https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/ParallelCG.cpp#L49. But perhaps there's a better way to factor this out.

I see where it is resetting MergedModule at line 608 in that patch, but not where it is used afterwards. It seems a bit fragile to me that the MergedModule would be valid when parallel code gen is off and not when it is. Does anything break if MergedModule is not set again after calling backend()?

fhahn added a comment.Jan 22 2021, 2:40 PM

Where/how is the returned module used in that case? How do you avoid having it accessed later in the parallel code gen case?

We should be able to use the returned module in the ParallelismLevel == 1 case, where we just pass a reference of the module to codegen. I think this behavior is needed for D94487 (llvm/lib/LTO/LTOCodeGenerator.cpp around line 608). This is a bit fragile/messy unfortunately, but I think we need it to preserve the existing libLTO behavior. It is also similar to the behavior of the previously used splitCodeGen https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/ParallelCG.cpp#L49. But perhaps there's a better way to factor this out.

I see where it is resetting MergedModule at line 608 in that patch, but not where it is used afterwards. It seems a bit fragile to me that the MergedModule would be valid when parallel code gen is off and not when it is.

Add to that that MergedModule is invalid while codegen is running....

Does anything break if MergedModule is not set again after calling backend()?

From the comments, it seems that is something that the API guarantees? @steven_wu do you know if writing back MergedModule is strictly necessary?

MergedModule is needed to support legacy API lto_codegen_write_merged_modules. You don't technical get the module back in memory, you just need to write to a file.

I forgot if there are APIs needed to work for merged module in memory but I will just implement that as first write to a file then read it back.

Offline comment to Florian is that:

The other possibility is that I don't see why SplitModule needs to own the module. Can backend just take Module& instead of grabbing the unique_ptr? That is probably the cleanest change.

fhahn updated this revision to Diff 319552.Jan 27 2021, 6:28 AM

Updated splitCodeGen & co to pass the module by reference instead of taking ownership. splitCodeGen clones the original module before carving it up, so it does not need ownership and it simplifies the code. We can change it back to taking ownership, once it is actually required.

fhahn retitled this revision from [LTOBackend] Return module when Parallelism Level == 1. to [LTO] Update splitCodeGen to take a reference to the module. (NFC).Jan 27 2021, 6:33 AM
fhahn edited the summary of this revision. (Show Details)
fhahn added a reviewer: pcc.

LGTM. @pcc does this look fine to you?

Updated splitCodeGen & co to pass the module by reference instead of taking ownership. splitCodeGen clones the original module before carving it up, so it does not need ownership and it simplifies the code. We can change it back to taking ownership, once it is actually required.

Will that cause issues if ownership is required down the line? If so, perhaps just remove the fixme about avoiding cloning the module for the last partition.

llvm/include/llvm/CodeGen/ParallelCG.h
34–35

Update comment

fhahn updated this revision to Diff 319877.Jan 28 2021, 8:20 AM

Updated splitCodeGen & co to pass the module by reference instead of taking ownership. splitCodeGen clones the original module before carving it up, so it does not need ownership and it simplifies the code. We can change it back to taking ownership, once it is actually required.

Will that cause issues if ownership is required down the line? If so, perhaps just remove the fixme about avoiding cloning the module for the last partition.

Yes, I think some additional changes will be needed to address the fixme. I think the fixme is still valuable, so rather than removing it I added a note about callers not expecting the module to change, so whoever addresses the fixme is aware that further changes are need. (The simplest fix would be to just clone the original module once).

What do you think?

fhahn marked an inline comment as done.Jan 28 2021, 8:20 AM
tejohnson added inline comments.Jan 28 2021, 8:48 AM
llvm/include/llvm/CodeGen/ParallelCG.h
35–39

I don't think this comment is accurate. Looking at splitCodeGen, in the case of parallelism=1 it will call codegen() on the input Module, which will apply passes to emit the Module, which in turn will modify it. In the case of parallelism>1, it will call SplitModule, which will externalize some symbols before doing the cloning (because the default of PreserveLocals is false).

fhahn updated this revision to Diff 319964.Jan 28 2021, 2:18 PM

Undo comment change.

fhahn added inline comments.Jan 28 2021, 2:19 PM
llvm/include/llvm/CodeGen/ParallelCG.h
35–39

Yes you are right, that wasn't right. I meant to say it preserves the semantics of the original module. I removed the comment.

This revision is now accepted and ready to land.Jan 28 2021, 2:33 PM
This revision was landed with ongoing or failed builds.Jan 29 2021, 3:53 AM
This revision was automatically updated to reflect the committed changes.