Option to ignore llvm[.compiler].used uses in hasAddressTaken()

[madhur13490]

llvm/lib/Analysis/CallGraph.cpp

85

Typo: Ingore

llvm/lib/IR/Function.cpp

1617

Can you please add a test which has non-bitcast value of function pointers? Something like below;

@llvm.used = appending global [2 x float()*] [void()* @foo, void()* @bar]

IIRC, the above simple cast to GlobalVariable is not sufficient in this case. You need to handle it with some more extra checks.

[rampitec]

Fixed typo.

[rampitec]

llvm/lib/IR/Function.cpp

1617

Does this really happen? My understanding these arrays shall have i8* type, so a function pointer always needs a bitcast.

[rampitec]

Handle the case when function pointer is used directly.

[dexonsmith]

(Passing comment not really related to the patch, but I wonder how hard / useful it would be to convert @llvm.used and @llvm.compiler.used to attributes (or similar)...?)

[rampitec]

In D96087#2552603, @dexonsmith wrote:

(Passing comment not really related to the patch, but I wonder how hard / useful it would be to convert @llvm.used and @llvm.compiler.used to attributes (or similar)...?)

At the very least these variables are defined in the LLVM IR specification, so it would be a compatibility break.

[dexonsmith]

In D96087#2552612, @rampitec wrote:

In D96087#2552603, @dexonsmith wrote:

(Passing comment not really related to the patch, but I wonder how hard / useful it would be to convert @llvm.used and @llvm.compiler.used to attributes (or similar)...?)

At the very least these variables are defined in the LLVM IR specification, so it would be a compatibility break.

Right, it'd certainly be a semantic change; we'd need an auto-upgrade for bitcode (surely no compatibility break there) and a transition of some sort for the C API (I imagine there's a reasonable way of handling both, but maybe you're seeing some difficulty?). Probably it could / should be modelled as part of the linkage somehow, rather than an attribute, but it might not be worth a bit on Value. (But probably this isn't the right venue to discuss this; carry on...)

[madhur13490]

llvm/lib/IR/Function.cpp

1582

Need to enhance this comment to accommodate new arguments.

1586

%s/Ingore/Ignore

1617

Yes, as per the spec, "This array contains a list of pointers to named global variables, functions and aliases which may optionally have a pointer cast formed of bitcast or getelementptr". So the pointer cast is optional. You can have plain function pointers in the array. The newly added code will handle only bitcast cast case but not general. You need to handle the general case and find if one of the users if GlobalVariable.

@llvm.used = appending global [2 x void()*] [void()* @foo, void()* @bar]

[rampitec]

Fixed typo.

llvm/lib/IR/Function.cpp

1617

This is already done in the previous patch. I still think that does not practically happen because you cannot have elements of different types in an array. The words "may optionally have a pointer cast" shall apply if your pointer is not of the i8* type (since this variable is not only used for functions). For sure one can artificially forge such initializer (like I did in the test), but that does not have much practical sense.

[madhur13490]

llvm/lib/IR/Function.cpp

1617

Two questions:

1. I don't understand why the below the initialization of llvm.used is invalid?

@llvm.used = appending global [2 x void()*] [void()* @foo, void()* @bar]

I don't see verifier complain about it, which make me believe that it is a valid LLVM construct. Are you saying that it is not common so you'd like to skip it?

2. Assumming that 1 is valid, does the patch succeed to determine if one of the user of function symbol is GlobalVariable?

[rampitec]

llvm/lib/IR/Function.cpp

1617

1. The initialization is not invalid, it just useless. If you have functions with different types in a module you will not be able to to initialize it. You also will be unable link different modules with such initializations.
2. Yet it works with the patch. See the test for llvm.compiler.used.

[madhur13490]

Accept with the condition that the comment is enhanced.

[rampitec]

Updated comment.

llvm/include/llvm/IR/Function.h

/// @}
 
   /// hasAddressTaken - returns true if there are any uses of this function
   /// other than direct calls or invokes to it, or blockaddress expressions.
   /// Optionally passes back an offending user for diagnostic purposes and
   /// ignores callback uses.
   ///
   bool hasAddressTaken(const User ** = nullptr,
                        bool IgnoreCallbackUses = false,
-                       bool IgnoreAssumeLikeCalls = false) const;
+                       bool IgnoreAssumeLikeCalls = false,
+                       bool IngoreLLVMUsed = false) const;
 
   /// isDefTriviallyDead - Return true if it is trivially safe to remove
   /// this function definition from the module (because it isn't externally
   /// visible, does not have its address taken, and has no callers).  To make
   /// this more accurate, call removeDeadConstantUsers first.
   bool isDefTriviallyDead() const;
 
   /// callsFunctionThatReturnsTwice - Return true if the function has a call to

llvm/lib/Analysis/CallGraph.cpp

 
 void CallGraph::addToCallGraph(Function *F) {
   CallGraphNode *Node = getOrInsertFunction(F);
 
   // If this function has external linkage or has its address taken and
   // it is not a callback, then anything could call it.
   if (!F->hasLocalLinkage() ||
       F->hasAddressTaken(nullptr, /*IgnoreCallbackUses=*/true,
-                         /* IgnoreAssumeLikeCalls */ true))
+                         /* IgnoreAssumeLikeCalls */ true,
+                         /* IgnoreLLVMUsed */ true))
     ExternalCallingNode->addCalledFunction(nullptr, Node);
 
   populateCallGraphNode(Node);
 }
 
 void CallGraph::populateCallGraphNode(CallGraphNode *Node) {
   Function *F = Node->getFunction();
 

llvm/lib/IR/Function.cpp

Optional<Function *> Intrinsic::remangleIntrinsicFunction(Function *F) {
   NewDecl->setCallingConv(F->getCallingConv());
   assert(NewDecl->getFunctionType() == F->getFunctionType() &&
          "Shouldn't change the signature");
   return NewDecl;
 }
 
 /// hasAddressTaken - returns true if there are any uses of this function
 /// other than direct calls or invokes to it. Optionally ignores callback
 /// uses.
 bool Function::hasAddressTaken(const User **PutOffender,
                                bool IgnoreCallbackUses,
-                               bool IgnoreAssumeLikeCalls) const {
+                               bool IgnoreAssumeLikeCalls,
+                               bool IngoreLLVMUsed) const {
   for (const Use &U : uses()) {
     const User *FU = U.getUser();
     if (isa<BlockAddress>(FU))
       continue;
 
     if (IgnoreCallbackUses) {
       AbstractCallSite ACS(&U);
       if (ACS && ACS.isCallbackCall())
         continue;
     }
 
     const auto *Call = dyn_cast<CallBase>(FU);
     if (!Call) {
       if (IgnoreAssumeLikeCalls) {
         if (const auto *FI = dyn_cast<Instruction>(FU)) {
           if (FI->isCast() && !FI->user_empty() &&
               llvm::all_of(FU->users(), [](const User *U) {
                 if (const auto *I = dyn_cast<IntrinsicInst>(U))
                   return I->isAssumeLikeIntrinsic();
                 return false;
               }))
             continue;
         }
       }
+      if (IngoreLLVMUsed && FU->hasOneUse() &&
+          !FU->user_begin()->user_empty() &&
+          llvm::all_of(FU->user_begin()->users(), [](const User *U) {
+            if (const auto *GV = dyn_cast<GlobalVariable>(U))
+              return GV->hasName() &&
+                     (GV->getName().equals("llvm.compiler.used") ||
+                      GV->getName().equals("llvm.used"));
+            return false;
+          })) {
+        continue;
+      }
       if (PutOffender)
         *PutOffender = FU;
       return true;
     }
     if (!Call->isCallee(&U)) {
       if (PutOffender)
         *PutOffender = FU;
       return true;

llvm/test/Analysis/CallGraph/ignore-llvm-used.ll

+; RUN: opt < %s -print-callgraph -disable-output 2>&1 | FileCheck %s
+; CHECK: Call graph node <<null function>><<{{.*}}>>  #uses=0
+; CHECK-NEXT:  CS<None> calls function 'bar'
+; CHECK-EMPTY:
+; CHECK-NEXT:   Call graph node for function: 'bar'<<{{.*}}>>  #uses=1
+; CHECK-EMPTY:
+; CHECK-NEXT:   Call graph node for function: 'foo'<<{{.*}}>>  #uses=0
+; CHECK-EMPTY:
+
[e-mail] = appending global [1 x i8*] [i8* bitcast (void ()* @foo to i8*)]
[e-mail] = appending global [1 x i8*] [i8* bitcast (void ()* @foo to i8*)]
+@array = appending global [1 x i8*] [i8* bitcast (void ()* @bar to i8*)]
+
+define internal void @foo() {
+entry:
+  ret void
+}
+
+define internal void @bar() {
+entry:
+  ret void
+}