Page MenuHomePhabricator

[Merge SImilar Function ThinLTO 3/n] Add hash code to function summary
Needs ReviewPublic

Authored by hiraditya on Oct 6 2018, 8:25 PM.

Diff Detail

Event Timeline

hiraditya created this revision.Oct 6 2018, 8:25 PM
hiraditya updated this revision to Diff 168581.Oct 6 2018, 9:23 PM
hiraditya retitled this revision from Add hash code to function summary to [Merge SImilar Function ThinLTO] Add hash code to function summary.Oct 13 2018, 10:44 PM
hiraditya edited the summary of this revision. (Show Details)Oct 13 2018, 11:05 PM
hiraditya retitled this revision from [Merge SImilar Function ThinLTO] Add hash code to function summary to [Merge SImilar Function ThinLTO 3/n] Add hash code to function summary.Oct 13 2018, 11:25 PM

Hi @hiraditya , pardon my ignorance in this part of llvm. Most of my comments are questions about your reasoning.

lib/Transforms/IPO/MergeSimilarFunctions.cpp
73

Why did you change these to externs?

89

Where is this function defined now?

92

Was this intentional, to add this diff with the entire profile function commented out?

1137

Maybe comment here that 0 is a placeholder/invalid hash?

1169

(same comment)

Yeah, I'm also wondering what's this for.

lib/Bitcode/Reader/BitcodeReader.cpp
5275

Are we no longer suggested to parse older index files?

tejohnson added inline comments.
lib/Transforms/IPO/MergeSimilarFunctions.cpp
1228

Where/how is this used by the patch? I don't see any use here. I tried finding in one of the other 4 related patches, but didn't see it being used there either.

hiraditya added inline comments.Oct 15 2018, 10:47 PM
lib/Transforms/IPO/MergeSimilarFunctions.cpp
92

Yes, the function definitions are now in ModuleSummaryAnalysis.cpp and the cl::opt is also referred there so I had to make them extern.

1137

Good point!

1228

I'll remove this one.

Remove ModuleSummaryIndex

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

port to llvm bitcode version 8