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

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

Does this line need to change?

186

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

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

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

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
55–57

exactly.

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

Lgtm.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
55–57

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.