EntryExitInstrumenter: Enable at all optimization levels (PR49143)

[zatrazz]

Summary

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

[aeubanks]

why is this now a module pass?

[zatrazz]

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.

[aeubanks]

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.

[zatrazz]

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.

[aeubanks]

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.

[zatrazz]

Updated patch based on previous comments.

[aeubanks]

code looks good, just some test nits

llvm/test/Transforms/EntryExitInstrumenter/mcount.ll

1

I don't think it makes sense to add tests to llvm/test, there should be a clang test for this since the pass is added in clang. These tests aren't really testing the changed code.
Are there any existing tests for CodeGenOpts.InstrumentFunctions? We just need to make sure that the calls to the instrument functions exist for clang -O0.

[aeubanks]

Alternatively: https://reviews.llvm.org/D97608

clang/lib/CodeGen/BackendUtil.cpp

if (!CodeGenOpts.DisableLLVMPasses) {
     if (!CodeGenOpts.ThinLTOIndexFile.empty())
       PB.registerPipelineStartEPCallback(
           [](ModulePassManager &MPM, PassBuilder::OptimizationLevel Level) {
             MPM.addPass(LowerTypeTestsPass(/*ExportSummary=*/nullptr,
                                            /*ImportSummary=*/nullptr,
                                            /*DropTypeTests=*/true));
           });
 
-    if (Level != PassBuilder::OptimizationLevel::O0) {
+    if (CodeGenOpts.InstrumentFunctions ||
+        CodeGenOpts.InstrumentFunctionsAfterInlining ||
+        CodeGenOpts.InstrumentFunctionEntryBare) {
       PB.registerPipelineStartEPCallback(
           [](ModulePassManager &MPM, PassBuilder::OptimizationLevel Level) {
             MPM.addPass(createModuleToFunctionPassAdaptor(
                 EntryExitInstrumenterPass(/*PostInlining=*/false)));
           });
     }
 
     // Register callbacks to schedule sanitizer passes at the appropriate part

llvm/include/llvm/Transforms/Utils/EntryExitInstrumenter.h

 
 #ifndef LLVM_TRANSFORMS_UTILS_ENTRYEXITINSTRUMENTER_H
 #define LLVM_TRANSFORMS_UTILS_ENTRYEXITINSTRUMENTER_H
 
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 
-class Function;
-
 struct EntryExitInstrumenterPass
     : public PassInfoMixin<EntryExitInstrumenterPass> {
   EntryExitInstrumenterPass(bool PostInlining) : PostInlining(PostInlining) {}
 
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 
   bool PostInlining;
+
+  static bool isRequired() { return true; }
 };
 
 } // namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_ENTRYEXITINSTRUMENTER_H

llvm/test/Transforms/EntryExitInstrumenter/mcount.ll

-; 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
 
 target datalayout = "E-m:e-i64:64-n32:64"
 target triple = "powerpc64le-unknown-linux"
 
 define void @leaf_function() #0 {
 entry:
   ret void
 
 ; CHECK-LABEL: define void @leaf_function()
 ; CHECK: entry:
 ; CHECK-NEXT: call void @mcount()
 ; CHECK-NEXT: %0 = call i8* @llvm.returnaddress(i32 0)
 ; CHECK-NEXT: call void @__cyg_profile_func_enter(i8* bitcast (void ()* @leaf_function to i8*), i8* %0)
 ; 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
 }
 
 
 define void @root_function() #0 {
 entry:
   call void @leaf_function()
   ret void
 
 ; CHECK-LABEL: define void @root_function()
 ; CHECK: entry:
 ; CHECK-NEXT: call void @mcount()
 
 ; CHECK-NEXT: %0 = call i8* @llvm.returnaddress(i32 0)
 ; CHECK-NEXT: call void @__cyg_profile_func_enter(i8* bitcast (void ()* @root_function to i8*), i8* %0)
 
 ; Entry and exit calls, inlined from @leaf_function()
 ; CHECK-NEXT: %1 = call i8* @llvm.returnaddress(i32 0)
 ; CHECK-NEXT: call void @__cyg_profile_func_enter(i8* bitcast (void ()* @leaf_function to i8*), i8* %1)
 ; CHECK-NEXT: %2 = call i8* @llvm.returnaddress(i32 0)
 ; CHECK-NEXT: call void @__cyg_profile_func_exit(i8* bitcast (void ()* @leaf_function to i8*), i8* %2)
 ; CHECK-NEXT: %3 = call i8* @llvm.returnaddress(i32 0)
 
 ; 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
 }
 
 
 
 ; The mcount function has many different names.
 
 define void @f1() #1 { entry: ret void }
 ; CHECK-LABEL: define void @f1