[memprof] Don't instrument PGO and other compiler inserted variables

[tejohnson]

Summary

Suppress instrumentation of PGO counter accesses, which is unnecessary
and costly. Also suppress accesses to other compiler inserted variables
starting with "__llvm". This is a slightly expanded variant of what is
done for tsan in shouldInstrumentReadWriteFromAddress.

[snehasish]

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

649

This changes the semantics from the previous version to only insert a load to the shadow memory if the function is instrumented. Is this expected?

The insertDynamicShadowAtFunctionEntry function unconditionally returns true. Would it be simpler to return early and then call it?

if (ToInstrument.empty()) {
  LLVM_DEBUG(dbgs() << "MEMPROF done instrumenting: " << FunctionModified << " "
                    << F << "\n");
 return FunctionModified;
}

FunctionModified = insertDynamicShadowAtFunctionEntry(F);

llvm/test/Instrumentation/HeapProfiler/skip-compiler-inserted.ll

5

I think this test will still pass if the memprof pass didn't run at all since there aren't any interesting loads/stores which we do instrument. Can you extend it to add another memory op which is instrumented by the pass?

[tejohnson]

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

649

This changes the semantics from the previous version to only insert a load to the shadow memory if the function is instrumented. Is this expected?

Yes - this change is mentioned at the end of the patch description. I realized we were inserting this unnecessarily when creating the new test case. However, with the change to the new test, this change is no longer tested here. I'm going to split it out into a different patch.

The insertDynamicShadowAtFunctionEntry function unconditionally returns true. Would it be simpler to return early and then call it?

I had considered doing an early return, but then I wasn't sure I wanted to duplicate the debug logging - but I went ahead and made this change since it is a little clearer. Left the "|=" in case we ever change insertDynamicShadowAtFunctionEntry to do this conditionally. (To be moved to a new patch)

[tejohnson]

Address comments. Remove now unrelated change.

[snehasish]

lgtm

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "memprof"
MemProfiler::isInterestingMemoryAccess(Instruction *I) const {
 
   // Ignore swifterror addresses.
   // swifterror memory addresses are mem2reg promoted by instruction
   // selection. As such they cannot have regular uses like an instrumentation
   // function and it makes no sense to track them as memory.
   if (Access.Addr->isSwiftError())
     return None;
 
+  // Peel off GEPs and BitCasts.
+  auto *Addr = Access.Addr->stripInBoundsOffsets();
+
+  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Addr)) {
+    // Do not instrument PGO counter updates.
+    if (GV->hasSection()) {
+      StringRef SectionName = GV->getSection();
+      // Check if the global is in the PGO counters section.
+      auto OF = Triple(I->getModule()->getTargetTriple()).getObjectFormat();
+      if (SectionName.endswith(
+              getInstrProfSectionName(IPSK_cnts, OF, /*AddSegmentInfo=*/false)))
+        return None;
+    }
+
+    // Do not instrument accesses to LLVM internal variables.
+    if (GV->getName().startswith("__llvm"))
+      return None;
+  }
+
   const DataLayout &DL = I->getModule()->getDataLayout();
   Access.TypeSize = DL.getTypeStoreSizeInBits(Access.AccessTy);
   return Access;
 }
 
 void MemProfiler::instrumentMaskedLoadOrStore(const DataLayout &DL, Value *Mask,
                                               Instruction *I, Value *Addr,
                                               unsigned Alignment,
bool MemProfiler::instrumentFunction(Function &F) {
   for (auto &BB : F) {
     for (auto &Inst : BB) {
       if (isInterestingMemoryAccess(&Inst) || isa<MemIntrinsic>(Inst))
         ToInstrument.push_back(&Inst);
     }
   }
 
   int NumInstrumented = 0;
   for (auto *Inst : ToInstrument) {
     if (ClDebugMin < 0 || ClDebugMax < 0 ||
         (NumInstrumented >= ClDebugMin && NumInstrumented <= ClDebugMax)) {
       Optional<InterestingMemoryAccess> Access =
           isInterestingMemoryAccess(Inst);
       if (Access)
         instrumentMop(Inst, F.getParent()->getDataLayout(), *Access);
       else
         instrumentMemIntrinsic(cast<MemIntrinsic>(Inst));

llvm/test/Instrumentation/HeapProfiler/skip-compiler-inserted.ll

+;; Test that we don't instrument loads to PGO counters or other
+;; compiler inserted variables.
+;
+; RUN: opt < %s -passes='function(memprof),module(memprof-module)' -S | FileCheck --check-prefixes=CHECK %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc__Z3foov = comdat nodeduplicate
+@__profc__Z3foov = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
+@__llvm_gcov_ctr = internal global [1 x i64] zeroinitializer
+
+define void @_Z3foov(i32* %a) {
+entry:
+  ;; Load that should get instrumentation.
+  %tmp1 = load i32, i32* %a, align 4
+  ;; PGO counter update
+  %pgocount = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3foov, i64 0, i64 0), align 8
+  %0 = add i64 %pgocount, 1
+  store i64 %0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3foov, i64 0, i64 0), align 8
+  ;; Gcov counter update
+  %gcovcount = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_gcov_ctr, i64 0, i64 0), align 8
+  %1 = add i64 %gcovcount, 1
+  store i64 %1, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_gcov_ctr, i64 0, i64 0), align 8
+  ret void
+}
+
+;; We should only add memory profile instrumentation for the first load.
+; CHECK: define void @_Z3foov
+; CHECK-NEXT: entry:
+; CHECK-NEXT:  %0 = load i64, i64* @__memprof_shadow_memory_dynamic_address, align 8
+; CHECK-NEXT:  %1 = ptrtoint i32* %a to i64
+; CHECK-NEXT:  %2 = and i64 %1, -64
+; CHECK-NEXT:  %3 = lshr i64 %2, 3
+; CHECK-NEXT:  %4 = add i64 %3, %0
+; CHECK-NEXT:  %5 = inttoptr i64 %4 to i64*
+; CHECK-NEXT:  %6 = load i64, i64* %5, align 8
+; CHECK-NEXT:  %7 = add i64 %6, 1
+; CHECK-NEXT:  store i64 %7, i64* %5, align 8
+; CHECK-NEXT:  %tmp1 = load i32, i32* %a, align 4
+; CHECK-NEXT:  %pgocount = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3foov, i64 0, i64 0)
+; CHECK-NEXT:  %8 = add i64 %pgocount, 1
+; CHECK-NEXT:  store i64 %8, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3foov, i64 0, i64 0)
+; CHECK-NEXT:  %gcovcount = load i64, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_gcov_ctr, i64 0, i64 0)
+; CHECK-NEXT:  %9 = add i64 %gcovcount, 1
+; CHECK-NEXT:  store i64 %9, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_gcov_ctr, i64 0, i64 0)
+; CHECK-NEXT:  ret void