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

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

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

500

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

508

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

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

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

Will do that. Thanks for pointing out.

500

yes, this is a leftover.

519

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

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

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