This is an archive of the discontinued LLVM Phabricator instance.

Imported statistics types changes
ClosedPublic

Authored by Prazek on Jul 29 2016, 4:30 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek updated this revision to Diff 66195.Jul 29 2016, 4:30 PM
Prazek retitled this revision from to Imported statistics types changes.
Prazek updated this object.
Prazek added reviewers: tejohnson, eraman.
Prazek added a subscriber: llvm-commits.
vsk added a subscriber: vsk.Jul 29 2016, 6:20 PM

Mostly looks fine, just two nitpicks.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
55 ↗(On Diff #66195)

Does this line need to change?

186 ↗(On Diff #66195)

It would probably be clearer to either change the const auto & to std::unique_ptr<...> & or to use Node.get() here.

Prazek marked 2 inline comments as done.Aug 1 2016, 2:53 PM
Prazek added inline comments.
lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
55 ↗(On Diff #66195)

Yes, because the map stores std::string of functions name. The name from Function can dissapear because the Function can dissapear.
Added extra comment to make it clear.

186 ↗(On Diff #66195)

The type is different than you think. It is StringMapEntry, so I am pushing pointers to StringMapEntry.
I added type to make it more clear.

tejohnson added inline comments.Aug 1 2016, 3:13 PM
lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
55 ↗(On Diff #66195)

The Caller function wouldn't have disappeared by here. The reason it needed to change is that NonImportedCallers was changed to store StringRef instead of a std::string, and we can use the StringRef owned by NodesMap to avoid creating a new copy of the string.

Prazek updated this revision to Diff 66393.Aug 1 2016, 3:26 PM
Prazek marked 2 inline comments as done.

Addressed comments

Prazek marked 2 inline comments as done.Aug 1 2016, 3:29 PM
Prazek added inline comments.
lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
52–59 ↗(On Diff #66393)

exactly.

vsk added a comment.Aug 1 2016, 3:34 PM

Lgtm.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
52–59 ↗(On Diff #66393)

Thanks for explaining

tejohnson accepted this revision.Aug 1 2016, 3:42 PM
tejohnson edited edge metadata.

LGTM. Thanks for the cleanup!

This revision is now accepted and ready to land.Aug 1 2016, 3:42 PM
This revision was automatically updated to reflect the committed changes.
Prazek marked an inline comment as done.