Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BOLT] [Passes] Fix two compile warnings in BOLT #73086

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

gz83
Copy link
Contributor

@gz83 gz83 commented Nov 22, 2023

Fix build issue on Windows.

issue:#73085

@maksfb PTAL thank you

Copy link

github-actions bot commented Nov 22, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@gz83
Copy link
Contributor Author

gz83 commented Nov 22, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 2c875719c841ff13b9b250e6ea97fc3e0aca2070 39d188b197b165898a3f2caaa819abdcf646c17d -- bolt/lib/Passes/IndirectCallPromotion.cpp bolt/lib/Passes/ReorderAlgorithm.cpp

View the diff from clang-format here.

diff --git a/bolt/lib/Passes/IndirectCallPromotion.cpp b/bolt/lib/Passes/IndirectCallPromotion.cpp
index 555258f072..260b96ba02 100644
--- a/bolt/lib/Passes/IndirectCallPromotion.cpp
+++ b/bolt/lib/Passes/IndirectCallPromotion.cpp
@@ -410,7 +410,8 @@ IndirectCallPromotion::maybeGetHotJumpTableTargets(BinaryBasicBlock &BB,
            << "BaseReg = " << BC.MRI->getName(BaseReg) << ", "
            << "IndexReg = " << BC.MRI->getName(IndexReg) << ", "
            << "DispValue = " << Twine::utohexstr(DispValue) << ", "
-           << "DispExpr = " << DispExpr << ", " << "MemLocInstr = ";
+           << "DispExpr = " << DispExpr << ", "
+           << "MemLocInstr = ";
     BC.printInstruction(dbgs(), *MemLocInstr, 0, &Function);
     dbgs() << "\n";
   });

I use clang-format -i -style=file bolt\lib\Passes\IndirectCallPromotion.cpp and clang-format -i -style=file bolt\lib\Passes\ReorderAlgorithm.cpp to format the code

@gz83
Copy link
Contributor Author

gz83 commented Dec 3, 2023

friendly ping

@@ -1467,7 +1466,7 @@ void IndirectCallPromotion::runOnFunctions(BinaryContext &BC) {
std::max<uint64_t>(TotalIndexBasedCandidates, 1))
<< "%\n";

(void)verifyProfile;
(void)verifyProfile(BFs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same. This will run verifyProfile(), which will cause an unnecessary slow down in no-debug mode. The previous code wasn't running anything, it was just making a reference to silence a compiler warning about verifyProfile being unused.

I believe the proper fix is actually to surround the definition of verifyProfile in #ifndef NDEBUG..#endif in the same way as it is use is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review. Do you mean I need to delete this line of code?

@rafaelauler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Delete the line entirely and surround the definition of verifyProfile with #ifndef, like that:

#ifndef NDEBUG
definition goes here
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github doesn't let me comment on unchanged code, but line 161 would look like this:

#ifndef NDEBUG
static bool verifyProfile(std::map<uint64_t, BinaryFunction> &BFs) {
  bool IsValid = true;
  for (auto &BFI : BFs) {
    BinaryFunction &BF = BFI.second;
    if (!BF.isSimple())
      continue;
    for (const BinaryBasicBlock &BB : BF) {
      auto BI = BB.branch_info_begin();
      for (BinaryBasicBlock *SuccBB : BB.successors()) {
        if (BI->Count != BinaryBasicBlock::COUNT_NO_PROFILE && BI->Count > 0) {
          if (BB.getKnownExecutionCount() == 0 ||
              SuccBB->getKnownExecutionCount() == 0) {
            errs() << "BOLT-WARNING: profile verification failed after ICP for "
                      "function "
                   << BF << '\n';
            IsValid = false;
          }
        }
        ++BI;
      }
    }
  }
  return IsValid;
}
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code has been modified, please review it, thank you!

@rafaelauler

Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gz83
Copy link
Contributor Author

gz83 commented Dec 6, 2023

Thanks!

Please help me merge it, thank you!

@rafaelauler

@rafaelauler rafaelauler merged commit fa5486e into llvm:main Dec 6, 2023
4 checks passed
@gz83 gz83 deleted the warnings branch December 6, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants