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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124703