Use CountingFunctionInserter both mcount and cygprofile calls, before and after inlining

[hans]

Summary

Clang implements the -finstrument-functions flag inherited from GCC, which inserts calls to __cyg_profile_func_{enter,exit} on function entry and exit.

This is useful for getting a trace of how the functions in a program are executed. Normally, the calls remain even if a function is inlined into another function, but it is useful to be able to turn this off for users who are interested in a lower-level trace, i.e. one that reflects what functions are called post-inlining. (We use this to generate link order files for Chromium.)

LLVM already has a pass for inserting similar instrumentation calls to mcount(), which it does after inlining. This patch renames and extends that pass to handle calls both to mcount and the cygprofile functions, before and/or after inlining as controlled by function attributes.

[hfinkel]

If you only want these calls post-inlining (and otherwise wish to minimize the effect on the rest of the optimizer), I'd recommend a different approach. What we did for mcount, which has similar requirements, was that we had the frontend add an attribute, and then a late pass actually added the function calls (lib/CodeGen/CountingFunctionInserter.cpp). I recommend extending CountingFunctionInserter to enable it to add these functions (based on some additional attribute), and then just add whatever attribute is necessary in the frontend based on some new flag.

[rnk]

In D39287#906565, @hfinkel wrote:

If you only want these calls post-inlining (and otherwise wish to minimize the effect on the rest of the optimizer), I'd recommend a different approach. What we did for mcount, which has similar requirements, was that we had the frontend add an attribute, and then a late pass actually added the function calls (lib/CodeGen/CountingFunctionInserter.cpp). I recommend extending CountingFunctionInserter to enable it to add these functions (based on some additional attribute), and then just add whatever attribute is necessary in the frontend based on some new flag.

+1. I know it's a big departure from how the existing feature is implemented, but it's consistent with every other major form of instrumentation that clang emits these days.

[hans]

In D39287#906565, @hfinkel wrote:

If you only want these calls post-inlining (and otherwise wish to minimize the effect on the rest of the optimizer),

As I mentioned in the description, the calls should generally be inserted before inlining and preserved. Doing them post-inlining should be an optional mode that I'm trying to add. This is sort of the crux of the problem, otherwise inserting the instrumentation later would be the obvious way to go.

I'd recommend a different approach. What we did for mcount, which has similar requirements, was that we had the frontend add an attribute, and then a late pass actually added the function calls (lib/CodeGen/CountingFunctionInserter.cpp). I recommend extending CountingFunctionInserter to enable it to add these functions (based on some additional attribute), and then just add whatever attribute is necessary in the frontend based on some new flag.

Would it be OK to run CountingFunctionInserter both before and after inlining and inserting these calls depending on some flag?

[hfinkel]

In D39287#907578, @hans wrote:

In D39287#906565, @hfinkel wrote:

If you only want these calls post-inlining (and otherwise wish to minimize the effect on the rest of the optimizer),

As I mentioned in the description, the calls should generally be inserted before inlining and preserved. Doing them post-inlining should be an optional mode that I'm trying to add. This is sort of the crux of the problem, otherwise inserting the instrumentation later would be the obvious way to go.

I understood. I figured that you would add them later in some mode. Otherwise, you would leave the logic as-is. As you suggest, you could have CountingFunctionInserter run both pre and post inlining and then not have duplicate logic.

I'd recommend a different approach. What we did for mcount, which has similar requirements, was that we had the frontend add an attribute, and then a late pass actually added the function calls (lib/CodeGen/CountingFunctionInserter.cpp). I recommend extending CountingFunctionInserter to enable it to add these functions (based on some additional attribute), and then just add whatever attribute is necessary in the frontend based on some new flag.

Would it be OK to run CountingFunctionInserter both before and after inlining and inserting these calls depending on some flag?

I'm not sure the logic of inserting the calls is complex enough to worry about reducing the duplication of having both logic in the frontend and in CountingFunctionInserter, but if you feel that it is, then I think this makes sense.

[hans]

Here's a new approach which renames and extends the existing CountingFunctionInserter to run both before and after inlining (with the old PM it's two passes actually) and handle both cygprofile and mcount calls as controlled by function attributes.

I haven't hooked up the Clang side yet, but I'd be interested to hear your thoughts on this approach.

[rnk]

I think this is the way to go. Thanks! :)

lib/Transforms/Utils/EntryExitInstrumenter.cpp

21–23 ↗

(On Diff #122360)

nit: mark these static, sink the anonymous namespace to the two pass structs.

56 ↗

(On Diff #122360)

nit: static

[hans]

Rebasing and addressing comments.

I've also added the pass in PassManagerBuilder::populateFunctionPassManager() because I realized that's how Clang sets up the passes.

One thing that worries me is that this set-up is pretty fragile against how the passes are run. I think it will currently work, but if the pass were to somehow run more than once, the instrumentation could get inserted multiple times. One way to avoid that would be to remove the attribute (or probably faster, give it an empty value?) when the pass "consumes" the attribute. Does that sound reasonable?

I also looked into consolidating this with the FEntryInserter pass which inserts calls to fentry. That turned out to be tricky however, because the calls are supposed to be inserted before the function prologue, which means the pass operates on MachineFunctions and inserts MachineInstrs. So I've left that out.

I've updated the Clang-side patch here: https://reviews.llvm.org/D39331

[rnk]

In D39287#922534, @hans wrote:

Rebasing and addressing comments.

I've also added the pass in PassManagerBuilder::populateFunctionPassManager() because I realized that's how Clang sets up the passes.

One thing that worries me is that this set-up is pretty fragile against how the passes are run. I think it will currently work, but if the pass were to somehow run more than once, the instrumentation could get inserted multiple times. One way to avoid that would be to remove the attribute (or probably faster, give it an empty value?) when the pass "consumes" the attribute. Does that sound reasonable?

What do other instrumentation passes do to defend against this? I guess they just don't have this problem because they don't appear twice in the pipeline?

I'm in favor of removing the function attribute when we instrument. AttributeLists are immutable, so nuking the whole attribute should be just as fast as setting the value.

I also looked into consolidating this with the FEntryInserter pass which inserts calls to fentry. That turned out to be tricky however, because the calls are supposed to be inserted before the function prologue, which means the pass operates on MachineFunctions and inserts MachineInstrs. So I've left that out.

Oh, wow, that's pretty special functionality. Seems best to leave it alone.

I've updated the Clang-side patch here: https://reviews.llvm.org/D39331

I think this is pretty uncontroversial, so let's remove the attribute on instrumentation, land this, and we can address any feedback from Hal or others in follow-ups.

[hans]

In D39287#923553, @rnk wrote:

In D39287#922534, @hans wrote:

Rebasing and addressing comments.

I've also added the pass in PassManagerBuilder::populateFunctionPassManager() because I realized that's how Clang sets up the passes.

One thing that worries me is that this set-up is pretty fragile against how the passes are run. I think it will currently work, but if the pass were to somehow run more than once, the instrumentation could get inserted multiple times. One way to avoid that would be to remove the attribute (or probably faster, give it an empty value?) when the pass "consumes" the attribute. Does that sound reasonable?

What do other instrumentation passes do to defend against this? I guess they just don't have this problem because they don't appear twice in the pipeline?

CountingFunctionInserter wouldn't appear in the pipeline twice becaues it runs so late, as part of the codegen passes (which is also why it lived under Codegen/). The sanitizer instrumentation passes avoid the problem by not being in the pipeline by default, but requiring e.g. Clang to insert them.

I'm in favor of removing the function attribute when we instrument. AttributeLists are immutable, so nuking the whole attribute should be just as fast as setting the value.

Done.

I've updated the Clang-side patch here: https://reviews.llvm.org/D39331

I think this is pretty uncontroversial, so let's remove the attribute on instrumentation, land this, and we can address any feedback from Hal or others in follow-ups.

Great, thanks. (I'll just wait until the Clang side is completely worked out)

lib/Analysis/InlineCost.cpp

static cl::opt<int> HintThreshold(
     "inlinehint-threshold", cl::Hidden, cl::init(325),
     cl::desc("Threshold for inlining functions with inline hint"));
 
 static cl::opt<int>
     ColdCallSiteThreshold("inline-cold-callsite-threshold", cl::Hidden,
                           cl::init(45),
                           cl::desc("Threshold for inlining cold callsites"));
 
+cl::opt<bool> DisableCygprofileInlining(
+    "disable-cygprofile-inlining",
+    cl::desc("Disable inlining of calls to __cyg_profile functions"),
+    cl::init(false));
+
 // We introduce this threshold to help performance of instrumentation based
 // PGO before we actually hook up inliner with analysis passes such as BPI and
 // BFI.
 static cl::opt<int> ColdThreshold(
     "inlinecold-threshold", cl::Hidden, cl::init(45),
     cl::desc("Threshold for inlining functions with cold attribute"));
 
 static cl::opt<int>

 
 bool CallAnalyzer::visitCallSite(CallSite CS) {
   if (CS.hasFnAttr(Attribute::ReturnsTwice) &&
       !F.hasFnAttribute(Attribute::ReturnsTwice)) {
     // This aborts the entire analysis.
     ExposesReturnsTwice = true;
     return false;
   }
+
   if (CS.isCall() && cast<CallInst>(CS.getInstruction())->cannotDuplicate())
     ContainsNoDuplicateCall = true;
 
   if (Function *F = CS.getCalledFunction()) {
+    // Ignore call instructions generated by -fprofile-functions. They will not
+    // be cloned, and should not affect the inlining decision.
+    if (DisableCygprofileInlining &&
+        (F->getName() == "__cyg_profile_func_enter" ||
+         F->getName() == "__cyg_profile_func_exit"))
+      return false;
+
     // When we have a concrete function, first try to simplify it directly.
     if (simplifyCallSite(F, CS))
       return true;
 
     // Next check if it is an intrinsic we know about.
     // FIXME: Lift this into part of the InstVisitor.
     if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction())) {
       switch (II->getIntrinsicID()) {

lib/Transforms/Utils/CloneFunction.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Utils/ValueMapper.h"
 #include <map>
 using namespace llvm;
 
+extern cl::opt<bool> DisableCygprofileInlining;
+
 /// See comments in Cloning.h.
 BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                                   const Twine &NameSuffix, Function *F,
                                   ClonedCodeInfo *CodeInfo,
                                   DebugInfoFinder *DIFinder) {
   DenseMap<const MDNode *, MDNode *> Cache;
   BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F);
   if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);

 /// anything that it can reach.
 void PruningFunctionCloner::CloneBlock(const BasicBlock *BB,
                                        BasicBlock::const_iterator StartingInst,
                                        std::vector<const BasicBlock*> &ToClone){
   WeakTrackingVH &BBEntry = VMap[BB];
 
   // Have we already cloned this block?
   if (BBEntry) return;
-  
+
   // Nope, clone it now.
   BasicBlock *NewBB;
   BBEntry = NewBB = BasicBlock::Create(BB->getContext());
   if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);
 
   // It is only legal to clone a function if a block address within that
   // function is never referenced outside of the function.  Given that, we
   // want to map block addresses from the old function to block addresses in
void PruningFunctionCloner::CloneBlock(const BasicBlock *BB,
   }
 
   bool hasCalls = false, hasDynamicAllocas = false, hasStaticAllocas = false;
 
   // Loop over all instructions, and copy them over, DCE'ing as we go.  This
   // loop doesn't include the terminator.
   for (BasicBlock::const_iterator II = StartingInst, IE = --BB->end();
        II != IE; ++II) {
+    // Drop call instructions generated by -fprofile-functions.
+    if (DisableCygprofileInlining) {
+      if (const CallInst *CI = dyn_cast<CallInst>(II)) {
+        if (const Function *Callee = CI->getCalledFunction()) {
+          if (Callee->getName() == "__cyg_profile_func_enter" ||
+              Callee->getName() == "__cyg_profile_func_exit")
+            continue;
+        }
+      }
+    }
 
     Instruction *NewInst = II->clone();
 
     // Eagerly remap operands to the newly cloned instruction, except for PHI
     // nodes for which we defer processing until we update the CFG.
     if (!isa<PHINode>(NewInst)) {
       RemapInstruction(NewInst, VMap,
                        ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);

test/Transforms/Inline/cygprofile-instrumentation.ll

+; RUN: opt < %s -inline -S | FileCheck %s --check-prefix=CHECK --check-prefix=INLINE
+; RUN: opt < %s -inline -inline-threshold=1 -disable-cygprofile-inlining -S | FileCheck %s --check-prefix=CHECK --check-prefix=NOINLINE
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+declare void @__cyg_profile_func_enter(i8*, i8*)
+declare void @__cyg_profile_func_exit(i8*, i8*)
+declare i8* @llvm.returnaddress(i32)
+
+define internal void @g() {
+entry:
+  %callsite = call i8* @llvm.returnaddress(i32 0)
+  call void @__cyg_profile_func_enter(i8* bitcast (void ()* @g to i8*), i8* %callsite)
+  %callsite1 = call i8* @llvm.returnaddress(i32 0)
+  call void @__cyg_profile_func_exit(i8* bitcast (void ()* @g to i8*), i8* %callsite1)
+  ret void
+}
+
+define void @f() {
+; CHECK-LABEL: @f
+; The call to @g should be inlined, but whether or not @g's cygprofile calls
+; should be inlined depends on the use of -disable-cygprofile-inlining.
+
+; CHECK: call void @__cyg_profile_func_enter
+; CHECK-NOT: call void @g()
+; INLINE: call void @__cyg_profile_func_enter
+; NOINLINE-NOT: call void @__cyg_profile_func_enter
+; CHECK: call void @__cyg_profile_func_exit
+; INLINE: call void @__cyg_profile_func_exit
+; NOINLINE-NOT: call void @__cyg_profile_func_exit
+
+entry:
+  %callsite = call i8* @llvm.returnaddress(i32 0)
+  call void @__cyg_profile_func_enter(i8* bitcast (void ()* @f to i8*), i8* %callsite)
+  call void @g()
+  %callsite1 = call i8* @llvm.returnaddress(i32 0)
+  call void @__cyg_profile_func_exit(i8* bitcast (void ()* @f to i8*), i8* %callsite1)
+  ret void
+}