[BOLT] fix print-mem-data not working

[Jianghibo]

This option print-mem-data is currently not working, use this fix to restore its functionality.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Haibo Jiang (Jianghibo)

Changes

This option print-mem-data is currently not working, use this fix to restore its functionality.

---

Full diff: https://github.com/llvm/llvm-project/pull/156332.diff

3 Files Affected:

• (modified) bolt/include/bolt/Core/MCPlusBuilder.h (+1-1)
• (modified) bolt/lib/Core/BinaryContext.cpp (+1-1)
• (modified) bolt/lib/Core/MCPlusBuilder.cpp (+5-2)

diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index ae04891e791f9..2bcddc82648b6 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -2190,7 +2190,7 @@ class MCPlusBuilder {
   }
 
   /// Print each annotation attached to \p Inst.
-  void printAnnotations(const MCInst &Inst, raw_ostream &OS) const;
+  void printAnnotations(const MCInst &Inst, raw_ostream &OS, bool PrintMemData = false) const;
 
   /// Remove annotation with a given \p Index.
   ///
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index dd0d041692484..e937751450ea8 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -2027,7 +2027,7 @@ void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction,
   if (MCSymbol *Label = MIB->getInstLabel(Instruction))
     OS << " # Label: " << *Label;
 
-  MIB->printAnnotations(Instruction, OS);
+  MIB->printAnnotations(Instruction, OS, PrintMemData || opts::PrintMemData);
 
   if (opts::PrintDebugInfo)
     printDebugInfo(OS, Instruction, Function, DwCtx.get());
diff --git a/bolt/lib/Core/MCPlusBuilder.cpp b/bolt/lib/Core/MCPlusBuilder.cpp
index 7f962e14ea115..c580c548eb05b 100644
--- a/bolt/lib/Core/MCPlusBuilder.cpp
+++ b/bolt/lib/Core/MCPlusBuilder.cpp
@@ -379,7 +379,7 @@ void MCPlusBuilder::stripAnnotations(MCInst &Inst, bool KeepTC) const {
 }
 
 void MCPlusBuilder::printAnnotations(const MCInst &Inst,
-                                     raw_ostream &OS) const {
+                                     raw_ostream &OS, bool PrintMemData) const {
   std::optional<unsigned> FirstAnnotationOp = getFirstAnnotationOpIndex(Inst);
   if (!FirstAnnotationOp)
     return;
@@ -390,7 +390,10 @@ void MCPlusBuilder::printAnnotations(const MCInst &Inst,
     const int64_t Value = extractAnnotationValue(Imm);
     const auto *Annotation = reinterpret_cast<const MCAnnotation *>(Value);
     if (Index >= MCAnnotation::kGeneric) {
-      OS << " # " << AnnotationNames[Index - MCAnnotation::kGeneric] << ": ";
+      std::string AnnotationName = AnnotationNames[Index - MCAnnotation::kGeneric];
+      if (!PrintMemData && AnnotationName == "MemoryAccessProfile")
+        continue;
+      OS << " # " << AnnotationName << ": ";
       Annotation->print(OS);
     }
   }

[github-actions]

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

[paschalis-mpeis]

Thanks for your patch @Jianghibo.

Could you add a small test to acompany the patch? Also see some comment below.

[paschalis-mpeis]

This change overrides annotation printing to consider the flag. Previously it looks like those annotations were always printed, but now would only be printed if PrintMemData is true?

[Jianghibo]

Yeah, if PrintMemData is enabled by default, then using a profile that includes memory access infomation will result in excessive printing of memory access details.

[Jianghibo]

Thanks for your patch @Jianghibo.

Could you add a small test to acompany the patch? Also see some comment below.

Added the test case. Since link_fdata cannot parse the memory access profile, I used an independent fdata to store memory access data.

[paschalis-mpeis]

Great, thanks for adding a test! Some minor improvements:

Could you use split-file to keep the .fdata lines in the same file as the code?

Also, for numbers like 0x137c810: are those absolute numbers from some other example?
Do they matter for this lit test? If not, we could use dummy values like:

4 main 10 4 otherSym 123 1
4 main 10 4 otherSym 456 2
4 main 10 4 otherSym abc 4

Maybe a comment right above the split-file region oculd explain that 10 (0x10) means attaching the mem profile 4 instructions from main.

[Jianghibo]

Great, thanks for adding a test! Some minor improvements:

Could you use split-file to keep the .fdata lines in the same file as the code?

Also, for numbers like 0x137c810: are those absolute numbers from some other example? Do they matter for this lit test? If not, we could use dummy values like:

4 main 10 4 otherSym 123 1
4 main 10 4 otherSym 456 2
4 main 10 4 otherSym abc 4

Maybe a comment right above the split-file region oculd explain that 10 (0x10) means attaching the mem profile 4 instructions from main.

Yeah, these values ​​are from other examples. Thanks for your comment, I have made the changes.

[paschalis-mpeis]

Looks good, thanks for the changes! Please allow a day or two before merging.

[paschalis-mpeis]

Merging as @Jianghibo does not have the necessary permissions.