[C++20] [Modules] Don't emit function bodies which is noinline and av…

[ChuanqiXu9]

…ailabl externally

A workaround for #60996

As the title suggested, we can avoid emitting available externally functions which is marked as noinline already. Such functions should contribute nothing for optimizations.

The update for docs will be sent seperately if this got approved.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang-modules

Changes

…ailabl externally

A workaround for #60996

As the title suggested, we can avoid emitting available externally functions which is marked as noinline already. Such functions should contribute nothing for optimizations.

The update for docs will be sent seperately if this god approved.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+3)
• (added) clang/test/Modules/no-import-func-body.cppm (+45)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index cae9dd93bc55921..eeb923a29f9056b 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -3847,6 +3847,9 @@ bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
     return false;
 
+  if (F->hasAttr<NoInlineAttr>())
+    return false;
+
   if (F->hasAttr<DLLImportAttr>() && !F->hasAttr<AlwaysInlineAttr>()) {
     // Check whether it would be safe to inline this dllimport function.
     DLLImportFunctionVisitor Visitor;
diff --git a/clang/test/Modules/no-import-func-body.cppm b/clang/test/Modules/no-import-func-body.cppm
new file mode 100644
index 000000000000000..9a111dca9855c09
--- /dev/null
+++ b/clang/test/Modules/no-import-func-body.cppm
@@ -0,0 +1,45 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -O1 -triple %itanium_abi_triple %t/a.cppm \
+// RUN:     -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -O1 -triple %itanium_abi_triple %t/b.cppm \
+// RUN:     -emit-module-interface -fprebuilt-module-path=%t -o %t/b.pcm
+// RUN: %clang_cc1 -std=c++20 -O1 -triple %itanium_abi_triple %t/c.cppm \
+// RUN:     -emit-module-interface -fprebuilt-module-path=%t -o %t/c.pcm
+// RUN: %clang_cc1 -std=c++20 -O1 -triple %itanium_abi_triple %t/c.pcm -S \
+// RUN:     -emit-llvm -disable-llvm-passes -o - | FileCheck %t/c.cppm
+
+//--- a.cppm
+export module a;
+export int a() {
+    return 43;
+}
+export __attribute__((noinline)) int a_noinline() {
+    return 44;
+}
+
+//--- b.cppm
+export module b;
+export import a;
+export int b() {
+    return 43 + a();
+}
+
+export __attribute__((noinline)) int b_noinline() {
+    return 43 + a();
+}
+
+//--- c.cppm
+export module c;
+export import b;
+export int c() {
+    return 43 + b() + a() + b_noinline() + a_noinline();
+}
+
+// CHECK: define{{.*}}available_externally{{.*}}@_ZW1b1bv(
+// CHECK: define{{.*}}available_externally{{.*}}@_ZW1a1av(
+
+// CHECK: declare{{.*}}@_ZW1b10b_noinlinev()
+// CHECK: declare{{.*}}@_ZW1a10a_noinlinev()

[davidstone]

This looks reasonable to me. I'll try to test this out by Monday evening or earlier to see what practical effect it has for some of my programs.

[dwblaikie]

This doesn't seem all that useful/important to me - a user can move the body of the function into an implementation unit rather than putting it in the interface unit and marking it noinline, right? This is the same recommendation we'd make if someone wrote a complex function definition in a header - and I think it's fine that the advice remains valid/relevant even in a modules build.

[philnik777]

This doesn't seem all that useful/important to me - a user can move the body of the function into an implementation unit rather than putting it in the interface unit and marking it noinline, right? This is the same recommendation we'd make if someone wrote a complex function definition in a header - and I think it's fine that the advice remains valid/relevant even in a modules build.

I think this makes a lot of sense for things like string, which requires all function definitions to be available for constexpr and template reasons, but we want to mark some functions as noinline for better optimizations. We haven't done that yet in libc++, but we'll have to at some point to address some regressions from making string constexpr in C++20.

[dwblaikie]

shrug I guess it's OK.

[ChuanqiXu9]

This doesn't seem all that useful/important to me - a user can move the body of the function into an implementation unit rather than putting it in the interface unit and marking it noinline, right? This is the same recommendation we'd make if someone wrote a complex function definition in a header - and I think it's fine that the advice remains valid/relevant even in a modules build.

It is helpful for some cases. e.g., we're lazy to create the implementation file. (I know it doesn't sound smart but such style exists. And the cost we pay in this patch are really little)

[iains]

I think it would be better to have a coherent plan to deal with the underlying issue; starting with separating the AST used for code-gen from that used for interfaces. Having said that, this LGTM (it will be interesting to see what performance gains are seen in practice).

[ChuanqiXu9]

Yeah, of course.