[CodeGen] make isTriviallyRecursive handle more trivial recursion

[george.burgess.iv]

Summary

This function was not catching all forms of trivial recursion, meaning:

void *memcpy(void *a, const void *b, size_t n) {
  return __builtin_memcpy(a, b, n);
}

would be considered trivially recursive, whereas

void *memcpy(void *a, const void *b, size_t n) {
  return memcpy(a, b, n);
}

would not.

[efriedma]

Could you go into a little more detail what problem you're trying to resolve?

isTriviallyRecursive is specifically a narrow hack to handle weird cases where a function is trying to hide the fact that it's calling itself, in ways that would convince gcc that the called function is different. If a function is actually explicitly calling itself, it's on the user to make sure they don't write an infinite loop, I think.

[george.burgess.iv]

Nothing in the real world :)

I was writing test-cases for a soon-to-be-in-your-inbox patch, and initially wrote memcpy recursion in terms of memcpy, rather than __builtin_memcpy. Wasn't sure if this was an oversight, or intended. The comments around the use of this through isTriviallyRecursive said "a[n externally-available] function that calls itself is clearly not equivalent to the real implementation." Since this behavior is intentional, I'll see if I can make that commentary a bit more clear instead.

Thanks!

clang/lib/CodeGen/CodeGenModule.cpp

if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(RT->getDecl()))
         return true;
 
   return false;
 }
 
 namespace {
   struct FunctionIsDirectlyRecursive
       : public ConstStmtVisitor<FunctionIsDirectlyRecursive, bool> {
+    const FunctionDecl *OriginalFD;
     const StringRef Name;
     const Builtin::Context &BI;
-    FunctionIsDirectlyRecursive(StringRef N, const Builtin::Context &C)
-        : Name(N), BI(C) {}
+    FunctionIsDirectlyRecursive(const FunctionDecl *OriginalFD, StringRef N,
+                                const Builtin::Context &C)
+        : OriginalFD(OriginalFD), Name(N), BI(C) {}
 
     bool VisitCallExpr(const CallExpr *E) {
       const FunctionDecl *FD = E->getDirectCallee();
       if (!FD)
         return false;
+      if (FD == OriginalFD)
+        return true;
       AsmLabelAttr *Attr = FD->getAttr<AsmLabelAttr>();
       if (Attr && Name == Attr->getLabel())
         return true;
       unsigned BuiltinID = FD->getBuiltinID();
       if (!BuiltinID || !BI.isLibFunction(BuiltinID))
         return false;
       StringRef BuiltinName = BI.getName(BuiltinID);
       if (BuiltinName.startswith("__builtin_") &&
if (getCXXABI().getMangleContext().shouldMangleDeclName(FD)) {
     AsmLabelAttr *Attr = FD->getAttr<AsmLabelAttr>();
     if (!Attr)
       return false;
     Name = Attr->getLabel();
   } else {
     Name = FD->getName();
   }
 
-  FunctionIsDirectlyRecursive Walker(Name, Context.BuiltinInfo);
+  FunctionIsDirectlyRecursive Walker(FD, Name, Context.BuiltinInfo);
   const Stmt *Body = FD->getBody();
   return Body ? Walker.Visit(Body) : false;
 }
 
 bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   if (getFunctionLinkage(GD) != llvm::Function::AvailableExternallyLinkage)
     return true;
   const auto *F = cast<FunctionDecl>(GD.getDecl());

clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang doesn't emit trivially-recursive extern inline functions.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit any available_externally function in
+// -O0.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+    __attribute__((gnu_inline))
+
+void some_other_fn(void);
+
+AVAILABLE_EXTERNALLY void *some_fn() {
+  some_other_fn();
+  return some_fn();
+}
+
+// CHECK-LABEL: define void @foo
+void foo() {
+  // CHECK-NOT: call void @some_other_fn
+  some_fn();
+}