Page MenuHomePhabricator

[Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported
Needs ReviewPublic

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

Details

Summary

Import the similar functions into the module with another function matching the hash.
Depends on: https://reviews.llvm.org/D53253

Diff Detail

Event Timeline

hiraditya created this revision.Oct 13 2018, 11:23 PM
hiraditya retitled this revision from [Merge SImilar Function ThinLTO] Set up similar function to be imported to [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported.Oct 13 2018, 11:26 PM

Hi @hiraditya , comments inline.

lib/Transforms/IPO/FunctionImport.cpp
488 ↗(On Diff #169610)

ooh boy this is a dense section of code. Possibly consider refactoring into helper functions for readability?

tejohnson added inline comments.Oct 15 2018, 10:21 AM
lib/Transforms/IPO/FunctionImport.cpp
488 ↗(On Diff #169610)

This is a good suggestion. Also, some high level comments describing what this code is doing/how it works would be helpful.

500 ↗(On Diff #169610)

Seems like this should be an actual assert. Actually, there is such an assert below, why the early return - leftover from debugging?

508 ↗(On Diff #169610)

You should fix this, and it should be straightforward. See where we check the return value of the ImportList insertion for importing above (variable PreviouslyImported).

512 ↗(On Diff #169610)

This could be avoided (and save an expensive hash lookup), if the SimFunctions saved the VIs to start with (although I haven't yet looked through your other patches to see how this is being populated).

519 ↗(On Diff #169610)

Why this constraint? Note that any global variables being referenced by an imported function will automatically have their declarations imported too.

hiraditya added inline comments.Oct 15 2018, 9:11 PM
lib/Transforms/IPO/FunctionImport.cpp
488 ↗(On Diff #169610)

Will do that. Thanks for pointing out.

500 ↗(On Diff #169610)

yes, this is a leftover.

519 ↗(On Diff #169610)

I'll remove this comment. Does not seem to be relevant.

tejohnson added inline comments.Oct 16 2018, 6:18 AM
lib/Transforms/IPO/FunctionImport.cpp
519 ↗(On Diff #169610)

It isn't just the comment, but also the corresponding check just below that the refs() be empty.

tejohnson added inline comments.Oct 16 2018, 8:08 AM
lib/Transforms/IPO/FunctionImport.cpp
519 ↗(On Diff #169610)

Note though that in its place you need to ensure that the function is eligible to import (notEligibleToImport() on the function summary), and you should also skip anything that was found to be globally unreachable (isGlobalValueLive() on the index). Not sure about checking whether it has interposable linkage. See some of the checks being done in selectCallee() in this file. Some of those are profitability (related to whether we can inline the imported function), but others are correctness checks. I.e. if it is notEligibleToImport then it might be referencing functions/variables that cannot be promoted to global scope for some reason.

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