Page MenuHomePhabricator

[Merge SImilar Function ThinLTO 4/n] Make merge function decisions before the thin-lto stage
Needs ReviewPublic

Authored by hiraditya on Oct 13 2018, 11:04 PM.

Details

Summary

Make decisions about which functions are to be imported, and the module of the function which will be hosting all the other functions.

Depends on: https://reviews.llvm.org/D52966

Diff Detail

Event Timeline

hiraditya created this revision.Oct 13 2018, 11:04 PM
hiraditya edited the summary of this revision. (Show Details)Oct 13 2018, 11:05 PM
hiraditya retitled this revision from Make merge function decisions before the thin-lto stage to [Merge SImilar Function ThinLTO] Make merge function decisions before the thin-lto stage.Oct 13 2018, 11:21 PM
hiraditya retitled this revision from [Merge SImilar Function ThinLTO] Make merge function decisions before the thin-lto stage to [Merge SImilar Function ThinLTO 4/n] Make merge function decisions before the thin-lto stage.Oct 13 2018, 11:25 PM

compiler error

Hi @hiraditya , comments inline.

lib/Transforms/IPO/MergeSimilarFunctions.cpp
1327 ↗(On Diff #169611)

Possibly convert this and the previous line to:

assert(!SimFunctions.count(0) && "Zero hash entries are placeholders");

It's hard to review the changes the way they are currently split up. E.g. This patch adds some fields to the index, without using them or testing them. Those changes are in a different patch or two. It would be good to isolate all the changes that create and set these new index fields and test them via dumping the index to assembly and looking for expected values. Then have another patch that uses them in the FunctionImporter. Etc.

Also, it looks like the changes are only enabled for:

  • the old pass manager
  • the old LTO API (lib/LTO/ThinLTOCodeGenerator.cpp).

Please also add to the new pass manager and the new LTO API (see LTO/LTO.cpp) at the same time.

include/llvm/Transforms/IPO/WholeProgramDevirt.h
234 ↗(On Diff #169611)

This isn't the right file to declare this function, which is unrelated to WPD.

Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2019, 10:34 PM