[CallGraph][FIX] Ensure generic intrinsics are represented in the CG

jdoerfert · jdoerfert · commit 10410534696e · 2023-01-10T11:38:58.000-08:00
Intrinsics have historically been excluded from the call graph with an exception of 3 special ones added at some point. This meant that passes depending on the call graph needed to handle intrinsics explicitly as the underlying assumption, namely that intrinsics can't call or modify things, doesn't hold. We are slowly moving away from special handling of intrinsics, or at least towards explicitly checking what intrinsics we want to handle differently. This patch: - Includes most intrinsics in the call graph. Debug intrinsics are still excluded. - Removes the special handling of intrinsics in the GlobalsAA pass. - Removes the `IntrinsicInst::isLeaf` method. Properly Fixes: #52706 See also: https://discourse.llvm.org/t/intrinsics-are-not-special-stop-pretending-i-mean-it/67545 Differential Revision: https://reviews.llvm.org/D14119
diff --git a/llvm/include/llvm/Analysis/CallGraph.h b/llvm/include/llvm/Analysis/CallGraph.h
@@ -240,9 +240,6 @@ class CallGraphNode {
 
   /// Adds a function to the list of functions called by this one.
   void addCalledFunction(CallBase *Call, CallGraphNode *M) {
-    assert(!Call || !Call->getCalledFunction() ||
-           !Call->getCalledFunction()->isIntrinsic() ||
-           !Intrinsic::isLeaf(Call->getCalledFunction()->getIntrinsicID()));
     CalledFunctions.emplace_back(Call ? std::optional<WeakTrackingVH>(Call)
                                       : std::optional<WeakTrackingVH>(),
                                  M);
diff --git a/llvm/include/llvm/IR/Intrinsics.h b/llvm/include/llvm/IR/Intrinsics.h
@@ -79,11 +79,6 @@ namespace Intrinsic {
   /// Returns true if the intrinsic can be overloaded.
   bool isOverloaded(ID id);
 
-  /// Returns true if the intrinsic is a leaf, i.e. it does not make any calls
-  /// itself.  Most intrinsics are leafs, the exceptions being the patchpoint
-  /// and statepoint intrinsics. These call (or invoke) their "target" argument.
-  bool isLeaf(ID id);
-
   /// Return the attributes for an intrinsic.
   AttributeList getAttributes(LLVMContext &C, ID id);
 
diff --git a/llvm/lib/Analysis/CallGraph.cpp b/llvm/lib/Analysis/CallGraph.cpp
@@ -14,7 +14,6 @@
 #include "llvm/IR/AbstractCallSite.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IntrinsicInst.h"
-#include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/InitializePasses.h"
@@ -92,20 +91,17 @@ void CallGraph::populateCallGraphNode(CallGraphNode *Node) {
 
   // If this function is not defined in this translation unit, it could call
   // anything.
-  if (F->isDeclaration() && !F->isIntrinsic())
+  if (F->isDeclaration() && !F->hasFnAttribute(Attribute::NoCallback))
     Node->addCalledFunction(nullptr, CallsExternalNode.get());
 
   // Look for calls by this function.
   for (BasicBlock &BB : *F)
     for (Instruction &I : BB) {
       if (auto *Call = dyn_cast<CallBase>(&I)) {
         const Function *Callee = Call->getCalledFunction();
-        if (!Callee || !Intrinsic::isLeaf(Callee->getIntrinsicID()))
-          // Indirect calls of intrinsics are not allowed so no need to check.
-          // We can be more precise here by using TargetArg returned by
-          // Intrinsic::isLeaf.
+        if (!Callee)
           Node->addCalledFunction(Call, CallsExternalNode.get());
-        else if (!Callee->isIntrinsic())
+        else if (!isDbgInfoIntrinsic(Callee->getIntrinsicID()))
           Node->addCalledFunction(Call, getOrInsertFunction(Callee));
 
         // Add reference to callback functions.
diff --git a/llvm/lib/Analysis/CallGraphSCCPass.cpp b/llvm/lib/Analysis/CallGraphSCCPass.cpp
@@ -267,15 +267,7 @@ bool CGPassManager::RefreshCallGraph(const CallGraphSCC &CurSCC, CallGraph &CG,
           // If we've already seen this call site, then the FunctionPass RAUW'd
           // one call with another, which resulted in two "uses" in the edge
           // list of the same call.
-          Calls.count(Call) ||
-
-          // If the call edge is not from a call or invoke, or it is a
-          // intrinsic call, then the function pass RAUW'd a call with
-          // another value. This can happen when constant folding happens
-          // of well known functions etc.
-          (Call->getCalledFunction() &&
-           Call->getCalledFunction()->isIntrinsic() &&
-           Intrinsic::isLeaf(Call->getCalledFunction()->getIntrinsicID()))) {
+          Calls.count(Call)) {
         assert(!CheckingMode &&
                "CallGraphSCCPass did not update the CallGraph correctly!");
 
diff --git a/llvm/lib/Analysis/GlobalsModRef.cpp b/llvm/lib/Analysis/GlobalsModRef.cpp
@@ -23,7 +23,6 @@
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"
-#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/InitializePasses.h"
@@ -593,20 +592,8 @@ void GlobalsAAResult::AnalyzeCallGraph(CallGraph &CG, Module &M) {
 
         // We handle calls specially because the graph-relevant aspects are
         // handled above.
-        if (auto *Call = dyn_cast<CallBase>(&I)) {
-          if (Function *Callee = Call->getCalledFunction()) {
-            // The callgraph doesn't include intrinsic calls.
-            if (Callee->isIntrinsic()) {
-              if (isa<DbgInfoIntrinsic>(Call))
-                // Don't let dbg intrinsics affect alias info.
-                continue;
-
-              MemoryEffects Behaviour = AAResultBase::getMemoryEffects(Callee);
-              FI.addModRefInfo(Behaviour.getModRef());
-            }
-          }
+        if (auto *Call = dyn_cast<CallBase>(&I))
           continue;
-        }
 
         // All non-call instructions we use the primary predicates for whether
         // they read or write memory.
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
@@ -1499,18 +1499,6 @@ bool Intrinsic::isOverloaded(ID id) {
 #undef GET_INTRINSIC_OVERLOAD_TABLE
 }
 
-bool Intrinsic::isLeaf(ID id) {
-  switch (id) {
-  default:
-    return true;
-
-  case Intrinsic::experimental_gc_statepoint:
-  case Intrinsic::experimental_patchpoint_void:
-  case Intrinsic::experimental_patchpoint_i64:
-    return false;
-  }
-}
-
 /// This defines the "Intrinsic::getAttributes(ID id)" method.
 #define GET_INTRINSIC_ATTRIBUTES
 #include "llvm/IR/IntrinsicImpl.inc"
diff --git a/llvm/test/Analysis/CallGraph/ignore-assumelike-calls.ll b/llvm/test/Analysis/CallGraph/ignore-assumelike-calls.ll
@@ -10,19 +10,23 @@
 ; CHECK-EMPTY:
 ; CHECK-NEXT:   Call graph node for function: 'bitcast_only'<<{{.*}}>>  #uses=0
 ; CHECK-EMPTY:
-; CHECK-NEXT:   Call graph node for function: 'llvm.lifetime.start.p0'<<{{.*}}>>  #uses=1
+; CHECK-NEXT:   Call graph node for function: 'llvm.lifetime.start.p0'<<{{.*}}>>  #uses=3
 ; CHECK-EMPTY:
-; CHECK-NEXT:   Call graph node for function: 'llvm.memset.p0.i64'<<{{.*}}>>  #uses=1
+; CHECK-NEXT:   Call graph node for function: 'llvm.memset.p0.i64'<<{{.*}}>>  #uses=2
 ; CHECK-EMPTY:
-; CHECK-NEXT:   Call graph node for function: 'llvm.memset.p1.i64'<<{{.*}}>>  #uses=1
+; CHECK-NEXT:   Call graph node for function: 'llvm.memset.p1.i64'<<{{.*}}>>  #uses=2
 ; CHECK-EMPTY:
 ; CHECK-NEXT:   Call graph node for function: 'other_cast_intrinsic_use'<<{{.*}}>>  #uses=1
+; CHECK-NEXT:   CS<{{.*}}> calls function 'llvm.memset.p1.i64'
 ; CHECK-EMPTY:
 ; CHECK-NEXT:   Call graph node for function: 'other_intrinsic_use'<<{{.*}}>>  #uses=1
+; CHECK-NEXT:   CS<{{.*}}> calls function 'llvm.memset.p0.i64'
 ; CHECK-EMPTY:
 ; CHECK-NEXT:   Call graph node for function: 'used_by_lifetime'<<{{.*}}>>  #uses=0
+; CHECK-NEXT:   CS<{{.*}}> calls function 'llvm.lifetime.start.p0'
 ; CHECK-EMPTY:
 ; CHECK-NEXT:   Call graph node for function: 'used_by_lifetime_cast'<<{{.*}}>>  #uses=0
+; CHECK-NEXT:   CS<{{.*}}> calls function 'llvm.lifetime.start.p0'
 ; CHECK-EMPTY:
 
 define internal void @used_by_lifetime() {
diff --git a/llvm/test/Analysis/CallGraph/non-leaf-intrinsics.ll b/llvm/test/Analysis/CallGraph/non-leaf-intrinsics.ll
@@ -25,7 +25,13 @@ entry:
 ; CHECK:  CS<None> calls function 'f'
 
 ; CHECK: Call graph node for function: 'calls_patchpoint'
-; CHECK-NEXT:  CS<[[addr_1:[^>]+]]> calls external node
+; CS<{{.*}}> calls function 'llvm.experimental.patchpoint.void'
 
 ; CHECK: Call graph node for function: 'calls_statepoint'
-; CHECK-NEXT:  CS<[[addr_0:[^>]+]]> calls external node
+; CS<{{.*}}> calls function 'llvm.experimental.gc.statepoint.p0'
+
+; CHECK: Call graph node for function: 'llvm.experimental.gc.statepoint.p0'<<{{.*}}>>  #uses=2
+; CHECK-NEXT:  CS<[[addr_1:[^>]+]]> calls external node
+
+; CHECK: Call graph node for function: 'llvm.experimental.patchpoint.void'<<{{.*}}>>  #uses=2
+; CHECK-NEXT:  CS<[[addr_1:[^>]+]]> calls external node
diff --git a/llvm/test/Transforms/GVN/intrinsics_in_cg.ll b/llvm/test/Transforms/GVN/intrinsics_in_cg.ll
@@ -2,7 +2,6 @@
 ; RUN: opt < %s -passes='require<globals-aa>,gvn' -S | FileCheck %s
 
 ; Ensure we do not hoist the load over the call.
-; FIXME: Currently broken until D141190 or similar lands.
 
 @G1 = internal global i32 1
 @G2 = internal global i32 1
@@ -32,16 +31,13 @@ check:
 define i32 @indirect_intrinsic(i1 %c) {
 ; CHECK-LABEL: define {{[^@]+}}@indirect_intrinsic
 ; CHECK-SAME: (i1 [[C:%.*]]) {
-; CHECK-NEXT:    br i1 [[C]], label [[INIT:%.*]], label [[DOTCHECK_CRIT_EDGE:%.*]]
-; CHECK:       .check_crit_edge:
-; CHECK-NEXT:    [[V_PRE:%.*]] = load i32, ptr @G2, align 4
-; CHECK-NEXT:    br label [[CHECK:%.*]]
+; CHECK-NEXT:    br i1 [[C]], label [[INIT:%.*]], label [[CHECK:%.*]]
 ; CHECK:       init:
 ; CHECK-NEXT:    store i32 0, ptr @G2, align 4
 ; CHECK-NEXT:    br label [[CHECK]]
 ; CHECK:       check:
-; CHECK-NEXT:    [[V:%.*]] = phi i32 [ [[V_PRE]], [[DOTCHECK_CRIT_EDGE]] ], [ 0, [[INIT]] ]
 ; CHECK-NEXT:    call void @intrinsic_caller()
+; CHECK-NEXT:    [[V:%.*]] = load i32, ptr @G2, align 4
 ; CHECK-NEXT:    ret i32 [[V]]
 ;
   br i1 %c, label %init, label %check
