This is an archive of the discontinued LLVM Phabricator instance.

[MemProf] Context disambiguation cloning pass [patch 4/4]
ClosedPublic

Authored by tejohnson on Apr 24 2023, 8:33 PM.

Details

Summary

Applies ThinLTO cloning decisions made during the thin link and
recorded in the summary index to the IR during the ThinLTO backend.

Depends on D141077.

Diff Detail

Event Timeline

tejohnson created this revision.Apr 24 2023, 8:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 8:33 PM
tejohnson requested review of this revision.Apr 24 2023, 8:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 8:33 PM
tejohnson updated this revision to Diff 516897.Apr 25 2023, 2:05 PM

Remove an extra set of identical test invocations left in by a merge.

snehasish accepted this revision.Apr 26 2023, 4:38 PM

lgtm

llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
3144

ore::NV instead to make it clear? (similar comment in D141077)

3211

nit: The Changed temporary var can be dropped here and below too.

if (ImportSummary) 
  return applyImport(M);
3251

The documentation discourages use of ExitOrError in libraries. Though I did see some uses in places which are not tools. You could add a static create method to raise the error outside the constructor but I'm not sure whats the convention of raising a fatal error from the Module pass.

[1] https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Error.h#L1353

This revision is now accepted and ready to land.Apr 26 2023, 4:38 PM
tejohnson marked 3 inline comments as done.May 4 2023, 9:29 AM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
3144

Done here and elsewhere with the NV calls added in this patch.

3251

Good catch. I had copied this approach from the WPD code which I guess is using the wrong mechanism, but I switched to using the mechanism used by FunctionImport.cpp which instead utilizes logAllUnhandledErrors. Also modified the control flow in this file to use early returns which I think makes it simpler to follow.

tejohnson updated this revision to Diff 519530.May 4 2023, 9:29 AM
tejohnson marked 2 inline comments as done.

Address comments

This revision was landed with ongoing or failed builds.May 5 2023, 4:26 PM
This revision was automatically updated to reflect the committed changes.