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 ↗(On Diff #169613)

Why did you change these to externs?

89 ↗(On Diff #169613)

Where is this function defined now?

92 ↗(On Diff #169613)

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

1137 ↗(On Diff #169613)

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

1169 ↗(On Diff #169613)

(same comment)

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

lib/Bitcode/Reader/BitcodeReader.cpp
5275 ↗(On Diff #169613)

Are we no longer suggested to parse older index files?

tejohnson added inline comments.
lib/Transforms/IPO/MergeSimilarFunctions.cpp
1228 ↗(On Diff #169613)

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 ↗(On Diff #169613)

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 ↗(On Diff #169613)

Good point!

1228 ↗(On Diff #169613)

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