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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemProfiler.cpp:647
+  if (!ToInstrument.empty())
+    FunctionModified |= insertDynamicShadowAtFunctionEntry(F);
+
----------------
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);
```




================
Comment at: llvm/test/Instrumentation/HeapProfiler/skip-compiler-inserted.ll:4
+;
+; RUN: opt < %s -passes='function(memprof),module(memprof-module)' -S | FileCheck --check-prefixes=CHECK %s
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124703/new/

https://reviews.llvm.org/D124703