This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][NFC] Replace stdio with raw_ostream in CallGraph
ClosedPublic

Authored by nhuhuan on Jun 1 2022, 2:14 PM.

Details

Summary

Replacing stdio functions, e.g., fopen, fprintf, with raw_ostream.

Test Plan:

ninja check-bolt

Diff Detail

Event Timeline

nhuhuan created this revision.Jun 1 2022, 2:14 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ayermolo. · View Herald Transcript
nhuhuan requested review of this revision.Jun 1 2022, 2:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 2:14 PM
Amir added a comment.Jun 1 2022, 3:08 PM

Looking good. Let's address some issues:

  1. Please retitle the diff as "[BOLT][NFC] Replace stdio with raw_ostream in CallGraph". The commit title should be a declarative statement, and explicit about where the change is made.
  2. While we're on it, let's add an option to use printDot method. Please use an example of PrintCFG option.
  3. Inline comments.
bolt/include/bolt/Passes/CallGraph.h
17

Are all of the new includes here really needed? Can you please prune the added includes as much as possible?

165
171

Please keep the type as is - there has been an effort to reduce the number of auto's in the codebase. LLVM coding style advises to be explicit about type in most cases: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

182–185

You can preserve the requested precision using the format method described here: https://llvm.org/doxygen/Format_8h_source.html (see comment on the top).

nhuhuan added inline comments.Jun 1 2022, 3:19 PM
bolt/include/bolt/Passes/CallGraph.h
17

I included cmath and iomanip to use std::fixed and std::setprecision to get right format for floating-point value.
But they are not compatible with raw_ostream, so I removed them, and forgot to remove cmath.
Including <string> is not necessary as it was already included in llvm/Support/FileSystem.h.
I will remove all of them in the next commit.

nhuhuan updated this revision to Diff 433597.Jun 1 2022, 5:29 PM

Prune unnecessary include, restrict use of auto keyword, format floating-point values.

nhuhuan retitled this revision from [BOLT][NFC] Replacing stdio functions with raw_ostream functions to [BOLT][NFC] Replace stdio with raw_ostream in CallGraph.Jun 1 2022, 5:31 PM
Amir added a comment.Jun 1 2022, 10:50 PM

Please address the review comments and mark them as resolved.

bolt/include/bolt/Passes/CallGraph.h
171

Let's keep iteration as is here and below

173–175

Let's drop the explicit type cast here and below.
I don't think it's needed given that

  • NodeId is an alias to size_t,
  • Nodes[].samples() returns uint64_t,
  • Nodes[].size() returns size_t.
177

Let's remove the added newline to make it easier to compare code side-by-side.

189

No need to explicitly close OS as the object is local to the scope and the fd is closed by destructor.

nhuhuan updated this revision to Diff 433657.Jun 1 2022, 11:21 PM
nhuhuan marked 7 inline comments as done.

Address new comments

nhuhuan marked an inline comment as done.Jun 1 2022, 11:23 PM
Amir added inline comments.Jun 2 2022, 12:06 AM
bolt/include/bolt/Passes/CallGraph.h
182–185

There are still some type casts left on this line

nhuhuan updated this revision to Diff 433672.Jun 2 2022, 12:30 AM

Remove remaining redundant type casts

nhuhuan marked an inline comment as done.Jun 2 2022, 12:33 AM
Amir accepted this revision.Jun 3 2022, 2:11 PM
This revision is now accepted and ready to land.Jun 3 2022, 2:11 PM
This revision was automatically updated to reflect the committed changes.