[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-18 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

In D96803#2571316 , @zatrazz wrote:

> In D96803#2569436 , @aeubanks wrote:
>
>> In D96803#2568179 , @zatrazz wrote:
>>
>>> In D96803#2566322 , @aeubanks 
>>> wrote:
>>>
 why is this now a module pass?
>>>
>>> Mainly to avoid the default rule from new pass manager to *not* apply any 
>>> FunctionPass for optnone (which is the main issue for PR49143). Is there a 
>>> better way to accomplish it? I noted also that 
>>> createModuleToFunctionPassAdaptor basically creates a adaptor that applies 
>>> the pass to all function on the module.
>>
>> It's always good to make the pass as specific as possible (e.g. prefer a 
>> function pass rather than a module pass) so it doesn't have to worry about 
>> infra. For example, just iterating over functions doesn't skip declarations.
>
> I modeled it after AlwaysInlinerPass, since it seems that it was enabled at 
> -O0 as well.

AlwaysInlinerPass is a module pass because it does interprocedural changes. 
EntryExitInstrumenterPass works on one function at a time, which should be 
modeled as a function pass.

> To keep EntryExitInstrumenterPass a function pass and enable it with -O0 I 
> think we will need a way to communicate to the pass manager that the pass 
> should be run even with optnone, my understanding was that a ModulePass will 
> the way to do it. Do we have a way to advertise that a function pass should 
> be run regardless of ' optnone' ?

That's what `isRequired()` is for...

Basically we need two changes. One is add `isRequired()` to 
EntryExitInstrumenterPass, the other is to run EntryExitInstrumenterPass under 
-O0 as well by removing the check in BackendUtil.cpp (or by changing it to 
`CodeGenOpts.InstrumentFunctions || ...` as you've done). Please keep the pass 
as a function pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96803

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-18 Thread Adhemerval Zanella via Phabricator via cfe-commits 
zatrazz added a comment.

In D96803#2569436 , @aeubanks wrote:

> In D96803#2568179 , @zatrazz wrote:
>
>> In D96803#2566322 , @aeubanks wrote:
>>
>>> why is this now a module pass?
>>
>> Mainly to avoid the default rule from new pass manager to *not* apply any 
>> FunctionPass for optnone (which is the main issue for PR49143). Is there a 
>> better way to accomplish it? I noted also that 
>> createModuleToFunctionPassAdaptor basically creates a adaptor that applies 
>> the pass to all function on the module.
>
> It's always good to make the pass as specific as possible (e.g. prefer a 
> function pass rather than a module pass) so it doesn't have to worry about 
> infra. For example, just iterating over functions doesn't skip declarations.

I modeled it after AlwaysInlinerPass, since it seems that it was enabled at -O0 
as well. To keep EntryExitInstrumenterPass a function pass and enable it with 
-O0 I think we will need a way to communicate to the pass manager that the pass 
should be run even with optnone, my understanding was that a ModulePass will 
the way to do it. Do we have a way to advertise that a function pass should be 
run regardless of ' optnone' ?

> The whole point of `isRequired()` is to make the pass always run when it's 
> added to the pipeline, so making it a module pass shouldn't be necessary with 
> that line.

Indeed, I have added to fix a testcase but it does seem unrequired. I will 
remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96803

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-17 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

In D96803#2568179 , @zatrazz wrote:

> In D96803#2566322 , @aeubanks wrote:
>
>> why is this now a module pass?
>
> Mainly to avoid the default rule from new pass manager to *not* apply any 
> FunctionPass for optnone (which is the main issue for PR49143). Is there a 
> better way to accomplish it? I noted also that 
> createModuleToFunctionPassAdaptor basically creates a adaptor that applies 
> the pass to all function on the module.

It's always good to make the pass as specific as possible (e.g. prefer a 
function pass rather than a module pass) so it doesn't have to worry about 
infra. For example, just iterating over functions doesn't skip declarations.

The whole point of `isRequired()` is to make the pass always run when it's 
added to the pipeline, so making it a module pass shouldn't be necessary with 
that line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96803

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-17 Thread Adhemerval Zanella via Phabricator via cfe-commits 
zatrazz added a comment.

In D96803#2566322 , @aeubanks wrote:

> why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any 
FunctionPass for optnone (which is the main issue for PR49143). Is there a 
better way to accomplish it? I noted also that 
createModuleToFunctionPassAdaptor basically creates a adaptor that applies the 
pass to all function on the module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96803

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-16 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

why is this now a module pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96803

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96803: EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143)

2021-02-16 Thread Adhemerval Zanella via Phabricator via cfe-commits 
zatrazz created this revision.
zatrazz added reviewers: lenary, ostannard, aeubanks.
Herald added a subscriber: hiraditya.
zatrazz requested review of this revision.
Herald added projects: clang, LLVM.

The function passes are disabled by default for optnone with the new
pass manager.  The pass is now enabled in clang backend if the
-finstrument-functions, -finstrument-function-entry-bare, or
-finstrument-functions-after-inlining instead of just for
optimization level different than -O0.

It fixes PR49143 and allows re-enable the eeprof-1.c test from
test-suite (disabled by D96521 ).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96803

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Transforms/Utils/EntryExitInstrumenter.h
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/test/Transforms/EntryExitInstrumenter/debug-info.ll
  llvm/test/Transforms/EntryExitInstrumenter/mcount.ll

Index: llvm/test/Transforms/EntryExitInstrumenter/mcount.ll
===
--- llvm/test/Transforms/EntryExitInstrumenter/mcount.ll
+++ llvm/test/Transforms/EntryExitInstrumenter/mcount.ll
@@ -1,7 +1,10 @@
-; RUN: opt -passes="function(ee-instrument),cgscc(inline),function(post-inline-ee-instrument)" -S < %s | FileCheck %s
+; RUN: opt --O0 --ee-instrument --inline --post-inline-ee-instrument -S < %s | FileCheck %s
+
+; Check if the pass is enabbled on both -O0 and higher optimization levels
+; RUN: opt --O2 --ee-instrument --post-inline-ee-instrument -S < %s | FileCheck --check-prefix=OPT %s
 
 ; Running the passes twice should not result in more instrumentation.
-; RUN: opt -passes="function(ee-instrument),function(ee-instrument),cgscc(inline),function(post-inline-ee-instrument),function(post-inline-ee-instrument)" -S < %s | FileCheck %s
+; RUN: opt -passes="ee-instrument,ee-instrument,cgscc(inline),post-inline-ee-instrument,post-inline-ee-instrument" -S < %s | FileCheck %s
 
 target datalayout = "E-m:e-i64:64-n32:64"
 target triple = "powerpc64le-unknown-linux"
@@ -18,6 +21,15 @@
 ; CHECK-NEXT: %1 = call i8* @llvm.returnaddress(i32 0)
 ; CHECK-NEXT: call void @__cyg_profile_func_exit(i8* bitcast (void ()* @leaf_function to i8*), i8* %1)
 ; CHECK-NEXT: ret void
+
+; OPT-LABEL: define void @leaf_function()
+; OPT: entry:
+; OPT-NEXT: call void @mcount()
+; OPT-NEXT: %0 = call i8* @llvm.returnaddress(i32 0)
+; OPT-NEXT: call void @__cyg_profile_func_enter(i8* bitcast (void ()* @leaf_function to i8*), i8* %0)
+; OPT-NEXT: %1 = call i8* @llvm.returnaddress(i32 0)
+; OPT-NEXT: call void @__cyg_profile_func_exit(i8* bitcast (void ()* @leaf_function to i8*), i8* %1)
+; OPT-NEXT: ret void
 }
 
 
@@ -42,6 +54,16 @@
 
 ; CHECK-NEXT: call void @__cyg_profile_func_exit(i8* bitcast (void ()* @root_function to i8*), i8* %3)
 ; CHECK-NEXT: ret void
+
+; OPT-LABEL: define void @root_function()
+; OPT: entry:
+; OPT-NEXT: call void @mcount()
+
+; OPT-NEXT: %0 = call i8* @llvm.returnaddress(i32 0)
+; OPT-NEXT: call void @__cyg_profile_func_enter(i8* bitcast (void ()* @root_function to i8*), i8* %0)
+; OPT-NEXT: %1 = call i8* @llvm.returnaddress(i32 0)
+; OPT-NEXT: call void @__cyg_profile_func_exit(i8* bitcast (void ()* @root_function to i8*), i8* %1)
+; OPT-NEXT: ret void
 }
 
 
Index: llvm/test/Transforms/EntryExitInstrumenter/debug-info.ll
===
--- llvm/test/Transforms/EntryExitInstrumenter/debug-info.ll
+++ llvm/test/Transforms/EntryExitInstrumenter/debug-info.ll
@@ -1,4 +1,4 @@
-; RUN: opt -passes="function(ee-instrument),cgscc(inline),function(post-inline-ee-instrument)" -S < %s | FileCheck %s
+; RUN: opt -passes="ee-instrument,cgscc(inline),post-inline-ee-instrument" -S < %s | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
Index: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
===
--- llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
+++ llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
@@ -176,9 +176,12 @@
   return new PostInlineEntryExitInstrumenter();
 }
 
-PreservedAnalyses
-llvm::EntryExitInstrumenterPass::run(Function , FunctionAnalysisManager ) {
-  runOnFunction(F, PostInlining);
+PreservedAnalyses llvm::EntryExitInstrumenterPass::run(Module ,
+   ModuleAnalysisManager ) {
+  for (Function  : M) {
+runOnFunction(F, PostInlining);
+  }
+
   PreservedAnalyses PA;
   PA.preserveSet();
   return PA;
Index: llvm/lib/Passes/PassRegistry.def
===
--- llvm/lib/Passes/PassRegistry.def
+++ llvm/lib/Passes/PassRegistry.def
@@ -120,6 +120,8 @@
 MODULE_PASS("memprof-module", ModuleMemProfilerPass())
 MODULE_PASS("poison-checking", PoisonCheckingPass())