[Memprof] Adds instrumentation support for memprof with histograms. #100834

mattweingarten · 2024-07-26T23:36:54Z

This patch allows running -fmemory-profile without the flag -memprof-use-callbacks, meaning the RecordAccessesHistogram is injected into IR as a sequence of instructions. This significantly increases performance of the instrumented binary.

llvmbot · 2024-07-26T23:37:26Z

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Matthew Weingarten (mattweingarten)

Changes

This patch allows running -fmemory-profile without the flag -memprof-use-callbacks, meaning the RecordAccessesHistogram is injected into IR as a sequence of instructions. This significantly increases performance of the instrumented binary.

Changes the HISTOGRAM_GRANULARITY from 8U to 8ULL to correct the shadow address translation.

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

2 Files Affected:

(modified) compiler-rt/lib/memprof/memprof_mapping.h (+1-1)
(modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+37-15)

diff --git a/compiler-rt/lib/memprof/memprof_mapping.h b/compiler-rt/lib/memprof/memprof_mapping.h
index fef8acfcfc921..6da385ab3d6e2 100644
--- a/compiler-rt/lib/memprof/memprof_mapping.h
+++ b/compiler-rt/lib/memprof/memprof_mapping.h
@@ -55,7 +55,7 @@ extern uptr kHighMemEnd; // Initialized in __memprof_init.
 // computed by summing up all individual 1 byte counters. This can incur an
 // accuracy penalty.
 
-#define HISTOGRAM_GRANULARITY 8U
+#define HISTOGRAM_GRANULARITY 8ULL
 
 #define HISTOGRAM_MAX_COUNTER 255U
 
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index 2c5d749d4a67a..0e4d21671d2a7 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -61,6 +61,9 @@ constexpr int LLVM_MEM_PROFILER_VERSION = 1;
 // Size of memory mapped to a single shadow location.
 constexpr uint64_t DefaultMemGranularity = 64;
 
+// Size of memory mapped to a single histogram bucket.
+constexpr uint64_t HistogramGranularity = 8;
+
 // Scale from granularity down to shadow size.
 constexpr uint64_t DefaultShadowScale = 3;
 
@@ -192,7 +195,8 @@ namespace {
 struct ShadowMapping {
   ShadowMapping() {
     Scale = ClMappingScale;
-    Granularity = ClMappingGranularity;
+    // Histogram Granularity is always 8U for now.
+    Granularity = ClHistogram ? HistogramGranularity : ClMappingGranularity;
     Mask = ~(Granularity - 1);
   }
 
@@ -288,10 +292,6 @@ ModuleMemProfilerPass::ModuleMemProfilerPass() = default;
 PreservedAnalyses ModuleMemProfilerPass::run(Module &M,
                                              AnalysisManager<Module> &AM) {
 
-  assert((!ClHistogram || (ClHistogram && ClUseCalls)) &&
-         "Cannot use -memprof-histogram without Callbacks. Set "
-         "memprof-use-callbacks");
-
   ModuleMemProfiler Profiler(M);
   if (Profiler.instrumentModule(M))
     return PreservedAnalyses::none();
@@ -489,16 +489,38 @@ void MemProfiler::instrumentAddress(Instruction *OrigIns,
     return;
   }
 
-  // Create an inline sequence to compute shadow location, and increment the
-  // value by one.
-  Type *ShadowTy = Type::getInt64Ty(*C);
-  Type *ShadowPtrTy = PointerType::get(ShadowTy, 0);
-  Value *ShadowPtr = memToShadow(AddrLong, IRB);
-  Value *ShadowAddr = IRB.CreateIntToPtr(ShadowPtr, ShadowPtrTy);
-  Value *ShadowValue = IRB.CreateLoad(ShadowTy, ShadowAddr);
-  Value *Inc = ConstantInt::get(Type::getInt64Ty(*C), 1);
-  ShadowValue = IRB.CreateAdd(ShadowValue, Inc);
-  IRB.CreateStore(ShadowValue, ShadowAddr);
+  if (ClHistogram) {
+    Type *ShadowTy = Type::getInt8Ty(*C);
+    Type *ShadowPtrTy = PointerType::get(ShadowTy, 0);
+    Value *Shadow = IRB.CreateAnd(AddrLong, Mapping.Mask);
+    Shadow = IRB.CreateLShr(Shadow, Mapping.Scale);
+    assert(DynamicShadowOffset);
+    Value *ShadowInteger = IRB.CreateAdd(Shadow, DynamicShadowOffset);
+
+    Value *ShadowAddr = IRB.CreateIntToPtr(ShadowInteger, ShadowPtrTy);
+    Value *ShadowValue = IRB.CreateLoad(ShadowTy, ShadowAddr);
+    Value *MaxCount = ConstantInt::get(Type::getInt8Ty(*C), 255);
+    Value *Cmp = IRB.CreateICmpULT(ShadowValue, MaxCount);
+    // Insert Basic block that increments histogram bucket if value is less
+    // than 255.
+    Instruction *IncBlock =
+        SplitBlockAndInsertIfThen(Cmp, InsertBefore, /*Unreachable=*/false);
+    IRB.SetInsertPoint(IncBlock);
+    Value *Inc = ConstantInt::get(Type::getInt8Ty(*C), 1);
+    ShadowValue = IRB.CreateAdd(ShadowValue, Inc);
+    IRB.CreateStore(ShadowValue, ShadowAddr);
+  } else {
+    // Create an inline sequence to compute shadow location, and increment the
+    // value by one.
+    Type *ShadowTy = Type::getInt64Ty(*C);
+    Type *ShadowPtrTy = PointerType::get(ShadowTy, 0);
+    Value *ShadowPtr = memToShadow(AddrLong, IRB);
+    Value *ShadowAddr = IRB.CreateIntToPtr(ShadowPtr, ShadowPtrTy);
+    Value *ShadowValue = IRB.CreateLoad(ShadowTy, ShadowAddr);
+    Value *Inc = ConstantInt::get(Type::getInt64Ty(*C), 1);
+    ShadowValue = IRB.CreateAdd(ShadowValue, Inc);
+    IRB.CreateStore(ShadowValue, ShadowAddr);
+  }
 }
 
 // Create the variable for the profile file name.

teresajohnson

Needs a test. E.g. see the instrumentation IR checking in llvm/test/Instrumentation/HeapProfiler/basic.ll.

teresajohnson · 2024-07-27T00:42:07Z

compiler-rt/lib/memprof/memprof_mapping.h

@@ -55,7 +55,7 @@ extern uptr kHighMemEnd; // Initialized in __memprof_init.
 // computed by summing up all individual 1 byte counters. This can incur an
 // accuracy penalty.

-#define HISTOGRAM_GRANULARITY 8U
+#define HISTOGRAM_GRANULARITY 8ULL


This should probably go in as a separate PR since I think it affects both versions?

Moved to PR: #100949.

Changed commit comment as well

I will keep the change in this PR for now until the other PR is merge, just so tests do not fail, but remove it before pushing.

teresajohnson · 2024-07-27T00:43:03Z

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

@@ -192,7 +195,8 @@ namespace {
 struct ShadowMapping {
  ShadowMapping() {
    Scale = ClMappingScale;
-    Granularity = ClMappingGranularity;
+    // Histogram Granularity is always 8U for now.


Not sure I understand the relevance of this comment

Oh are you explaining why it is not a cl::opt? That's sort of unclear

Yes that was what I was referring to. Removed comment since its not necessary.

Add an assert then if the user specifies both ClHistogram and ClMappingGranularity.

sounds good, done.

teresajohnson · 2024-07-27T00:54:58Z

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

+    Value *Shadow = IRB.CreateAnd(AddrLong, Mapping.Mask);
+    Shadow = IRB.CreateLShr(Shadow, Mapping.Scale);
+    assert(DynamicShadowOffset);
+    Value *ShadowInteger = IRB.CreateAdd(Shadow, DynamicShadowOffset);


Can't the above 4 lines just reuse memToShadow?

In fact, there is a lot of common stuff between the histogram and non-histogram cases. Can you just make a few pieces of it condition, such as the selection of the ShadowTy and the conditional handling?

Good point, fixed.

mattweingarten · 2024-07-28T23:16:28Z

Needs a test. E.g. see the instrumentation IR checking in llvm/test/Instrumentation/HeapProfiler/basic.ll.

Added basic-histogram.ll for testing

teresajohnson

lgtm with assert suggested in my last comment.

…grams. This patch allows running `-fmemory-profile` without `-memprof-use-callbacks`. This significantly increases performance of the instrumented binary. Additionally, changes the `HISTOGRAM_GRANULARITY` from 8U to 8ULL to correct shadow address translation.

llvmbot added compiler-rt PGO Profile Guided Optimizations llvm:transforms labels Jul 26, 2024

teresajohnson reviewed Jul 27, 2024

View reviewed changes

teresajohnson approved these changes Jul 29, 2024

View reviewed changes

mattweingarten force-pushed the MemprofHistogramInstrumentation branch from 07a5c65 to bfaf9c9 Compare July 29, 2024 19:49

mattweingarten force-pushed the MemprofHistogramInstrumentation branch from 56d14e1 to 32e8c99 Compare July 29, 2024 23:08

teresajohnson merged commit 17993eb into llvm:main Jul 29, 2024
4 of 6 checks passed

RKSimon mentioned this pull request Feb 17, 2025

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp:289:Redundant condition in assert #96862

Closed

[Memprof] Adds instrumentation support for memprof with histograms. #100834

[Memprof] Adds instrumentation support for memprof with histograms. #100834

Uh oh!

Conversation

mattweingarten commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattweingarten commented Jul 28, 2024

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattweingarten commented Jul 26, 2024 •

edited

Loading

llvmbot commented Jul 26, 2024 •

edited

Loading