[CIR] Fix calling defined functions

[andykaylor]

Until now our function symbol lookup has been assuming that the function did not exist and creating a definition for it. This caused us to create a duplicate definition if we ever tried to call a function that was already defined.

This change fixes that by adding handling for trying to look up existing global definitions before creating a new one.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Andy Kaylor (andykaylor)

Changes

Until now our function symbol lookup has been assuming that the function did not exist and creating a definition for it. This caused us to create a duplicate definition if we ever tried to call a function that was already defined.

This change fixes that by adding handling for trying to look up existing global definitions before creating a new one.

---

Full diff: https://github.com/llvm/llvm-project/pull/137271.diff

4 Files Affected:

• (modified) clang/include/clang/CIR/MissingFeatures.h (+1)
• (modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+52)
• (modified) clang/test/CIR/CodeGen/call.cpp (+6-5)
• (modified) clang/test/CIR/CodeGen/namespace.cpp (+1-4)

diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 045b9ce40f53a..bb5dac4faa1e0 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -160,6 +160,7 @@ struct MissingFeatures {
   static bool lambdaFieldToName() { return false; }
   static bool targetSpecificCXXABI() { return false; }
   static bool moduleNameHash() { return false; }
+  static bool setDSOLocal() { return false; }
 
   // Missing types
   static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index 0f4193b5756fd..e4c08328c59fd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -773,6 +773,58 @@ cir::FuncOp CIRGenModule::getOrCreateCIRFunction(
     StringRef mangledName, mlir::Type funcType, GlobalDecl gd, bool forVTable,
     bool dontDefer, bool isThunk, ForDefinition_t isForDefinition,
     mlir::ArrayAttr extraAttrs) {
+  const Decl *d = gd.getDecl();
+
+  if (isThunk)
+    errorNYI(d->getSourceRange(), "getOrCreateCIRFunction: thunk");
+
+  // In what follows, we continue past 'errorNYI' as if nothing happened because
+  // the rest of the implementation is better than doing nothing.
+
+  // Any attempts to use a MultiVersion function should result in retrieving the
+  // iFunc instead. Name mangling will handle the rest of the changes.
+  if (const auto *fd = cast_or_null<FunctionDecl>(d)) {
+    // For the device mark the function as one that should be emitted.
+    if (getLangOpts().OpenMPIsTargetDevice && fd->isDefined() && !dontDefer &&
+        !isForDefinition)
+      errorNYI(fd->getSourceRange(),
+               "getOrCreateCIRFunction: OpenMP target function");
+
+    if (fd->isMultiVersion())
+      errorNYI(fd->getSourceRange(), "multi-version functions NYI");
+  }
+
+  // Lookup the entry, lazily creating it if necessary.
+  mlir::Operation *entry = getGlobalValue(mangledName);
+  if (entry) {
+    if (!isa<cir::FuncOp>(entry))
+      errorNYI(d->getSourceRange(), "getOrCreateCIRFunction: non-FuncOp");
+
+    assert(!cir::MissingFeatures::weakRefReference());
+
+    // Handle dropped DLL attributes.
+    if (d && !d->hasAttr<DLLImportAttr>() && !d->hasAttr<DLLExportAttr>()) {
+      assert(!cir::MissingFeatures::setDLLStorageClass());
+      assert(!cir::MissingFeatures::setDSOLocal());
+    }
+
+    // If there are two attempts to define the same mangled name, issue an
+    // error.
+    auto fn = cast<cir::FuncOp>(entry);
+    assert((!isForDefinition || !fn || !fn.isDeclaration()) &&
+           "Duplicate function definition");
+    if (fn && fn.getFunctionType() == funcType) {
+      return fn;
+    }
+
+    if (!isForDefinition) {
+      return fn;
+    }
+
+    // TODO(cir): classic codegen checks here if this is a llvm::GlobalAlias.
+    // How will we support this?
+  }
+
   auto *funcDecl = llvm::cast_or_null<FunctionDecl>(gd.getDecl());
   bool invalidLoc = !funcDecl ||
                     funcDecl->getSourceRange().getBegin().isInvalid() ||
diff --git a/clang/test/CIR/CodeGen/call.cpp b/clang/test/CIR/CodeGen/call.cpp
index 84e773ec34586..b79abb0d46efb 100644
--- a/clang/test/CIR/CodeGen/call.cpp
+++ b/clang/test/CIR/CodeGen/call.cpp
@@ -1,19 +1,20 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 | FileCheck %s
 
-void f1();
+void f1() {}
 void f2() {
   f1();
 }
 
-// CHECK-LABEL: cir.func @_Z2f1v
+// CHECK-LABEL:  cir.func @_Z2f1v
+// CHECK-LABEL: cir.func @_Z2f2v
 // CHECK:         cir.call @_Z2f1v() : () -> ()
 
-int f3();
+int f3() { return 2; }
 int f4() {
   int x = f3();
   return x;
 }
 
+// CHECK-LABEL: cir.func @_Z2f3v() -> !s32i
 // CHECK-LABEL: cir.func @_Z2f4v() -> !s32i
-// CHECK:         %[[#x:]] = cir.call @_Z2f3v() : () -> !s32i
-// CHECK-NEXT:    cir.store %[[#x]], %{{.+}} : !s32i, !cir.ptr<!s32i>
+// CHECK:         cir.call @_Z2f3v() : () -> !s32i
diff --git a/clang/test/CIR/CodeGen/namespace.cpp b/clang/test/CIR/CodeGen/namespace.cpp
index cfeb17bd19ced..c89adcbfc7811 100644
--- a/clang/test/CIR/CodeGen/namespace.cpp
+++ b/clang/test/CIR/CodeGen/namespace.cpp
@@ -4,10 +4,7 @@
 namespace {
   int g1 = 1;
 
-  // Note: This causes a warning about the function being undefined, but we
-  // currently have a problem with duplicate definitions when we call functions.
-  // This should be updated when that problem is fixed.
-  void f1(void);
+  void f1(void) {}
 }
 
 

[erichkeane]

2 nits, else lgtm.

[erichkeane]

Is this comment out of place? Looks like it should be with the actual multiversion stuff on 793.

[andykaylor]

I think you're right. When this comment was originally written (by you, incidentally), line 793 immediately followed line 786, so the comment made sense here. When the OpenMP handling was added, the comment wasn't moved but it should have been. I'll fix that here. The classic codegen will still have this comment location issue.

[erichkeane]

Fixed in classic-codegen: 785ab45

[erichkeane]

Is the leading spaces weird here?