[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D148723#4283094 , 
@serge-sans-paille wrote:

> In D148723#4280916 , @efriedma 
> wrote:
>
>> The point of an "inline builtin" is that the inline function is actually the 
>> original function; it's just the inline implementation is only used in 
>> limited circumstances (in particular, it can't be used recursively).  
>> Changing the linkage could have unexpected side-effects.
>
> That's not my understanding. An inline builtin provides an alternate 
> implementation of the builtin that usually wraps the original builtin in some 
> way. It's not meant to be externally visible (it would probably collide with 
> libc's implementation in that case).

The "linkage" here is the linkage of the declaration.  Any code that refers to 
the declaration has to treat it like something externally visible.  Off the top 
of my head, not sure how much it matters for C, but the linkage at the AST 
level has cascading effects in C++.

For normal C99 inline functions, there's a definition somewhere else, but 
there's an inline body which is supposed to be equivalent.  Sort of a 
substitute for "linkonce", which doesn't normally exist in C.  The issue with 
builtins specifically is that glibc headers do weird things, and the inline 
version ends up recursively calling itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

I think something like

#ifdef RINZOCORE_SHARED
#define RINZO_LIB __declspec(dllexport)
#else
#define RINZO_LIB __declspec(dllimport)
#endif

RINZO_LIB inline func() {}


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11538
  const FunctionDecl *FD) {
-  if (!FD->isExternallyVisible())
+  if (!FD->isExternallyVisible() || FD->isInlineBuiltinDeclaration())
 return GVA_Internal;

serge-sans-paille wrote:
> jyu2 wrote:
> > How about __declspec(dllimpor/dllexprort) __forceinline fexprl?
> Out of curiosity : where does the failing code comes from? A standard library 
> or user-specific code? I assume standard library, in that case would you mind 
> sharing a link to the source? I'm trying to understand the meaning we would 
> like to give to an inline builtin with external linkage but force inline 
> attribute. 
I believe it comes from the version of math.h that we ship with MKL/our 
compiler in our downstream. We have our own separate implementation of much of 
math.h.

So you can pick it up from here: 
https://www.intel.com/content/www/us/en/developer/tools/oneapi/dpc-compiler.html
 (free download at least).




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[serge via Phabricator via cfe-commits]

serge-sans-paille added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5192
 
-  if (const auto *FD = D->getAsFunction())
+  if (const auto *FD = D->getAsFunction()) {
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)

erichkeane wrote:
> Is this an unrelated change?
indeed ^^!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[serge via Phabricator via cfe-commits]

serge-sans-paille added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11538
  const FunctionDecl *FD) {
-  if (!FD->isExternallyVisible())
+  if (!FD->isExternallyVisible() || FD->isInlineBuiltinDeclaration())
 return GVA_Internal;

jyu2 wrote:
> How about __declspec(dllimpor/dllexprort) __forceinline fexprl?
Out of curiosity : where does the failing code comes from? A standard library 
or user-specific code? I assume standard library, in that case would you mind 
sharing a link to the source? I'm trying to understand the meaning we would 
like to give to an inline builtin with external linkage but force inline 
attribute. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D148723#4280916 , @efriedma wrote:

> This seems like a weird way to fix this.

I agree, not a big fan either. But wanting to start the bike shedding in some 
way.

> The point of an "inline builtin" is that the inline function is actually the 
> original function; it's just the inline implementation is only used in 
> limited circumstances (in particular, it can't be used recursively).  
> Changing the linkage could have unexpected side-effects.

That's not my understanding. An inline builtin provides an alternate 
implementation of the builtin that usually wraps the original builtin in some 
way. It's not meant to be externally visible (it would probably collide with 
libc's implementation in that case).

> Maybe it makes sense to restore some form of the "GNUInlineAttr" check in 
> isInlineBuiltinDeclaration.  But instead of actually checking for the 
> attribute, check that the function would be emitted with available_externally 
> linkage.  So "inline builtins" don't exist on Windows because inline 
> functions are linkonce_odr.  But we still detect builtins that are declared 
> `inline` instead of `extern inline __attribute((gnu_inline))`.

I'll give this some thoughts / experiments. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This seems like a weird way to fix this.  The point of an "inline builtin" is 
that the inline function is actually the original function; it's just the 
inline implementation is only used in limited circumstances (in particular, it 
can't be used recursively).  Changing the linkage could have unexpected 
side-effects.

Maybe it makes sense to restore some form of the "GNUInlineAttr" check in 
isInlineBuiltinDeclaration.  But instead of actually checking for the 
attribute, check that the function would be emitted with available_externally 
linkage.  So "inline builtins" don't exist on Windows because inline functions 
are linkonce_odr.  But we still detect builtins that are declared `inline` 
instead of `extern inline __attribute((gnu_inline))`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5192
 
-  if (const auto *FD = D->getAsFunction())
+  if (const auto *FD = D->getAsFunction()) {
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)

Is this an unrelated change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

Hi @serge-sans-paille, thanks for the fix.  Could you please also try some test 
with dllimport/dllexport with inline function?  Jennifer




Comment at: clang/lib/AST/ASTContext.cpp:11538
  const FunctionDecl *FD) {
-  if (!FD->isExternallyVisible())
+  if (!FD->isExternallyVisible() || FD->isInlineBuiltinDeclaration())
 return GVA_Internal;

How about __declspec(dllimpor/dllexprort) __forceinline fexprl?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148723/new/

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added reviewers: jyu2, efriedma.
Herald added a project: All.
serge-sans-paille requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Otherwise we end up with link once odr because of the inline keyword, which 
translates to comdat in windows world. But we prune the body and that's 
inconsistent...
See https://godbolt.org/z/z9G87Wr37 for the original failing test case


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148723

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/inline-builtin-comdat.c


Index: clang/test/CodeGen/inline-builtin-comdat.c
===
--- /dev/null
+++ clang/test/CodeGen/inline-builtin-comdat.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-windows -S -emit-llvm %s -o - | FileCheck %s
+// Make sure we don't generate definition for Inline Builtins on windows
+
+double __cdecl frexp( double _X, int* _Y);
+inline __attribute__((always_inline))  long double __cdecl frexpl( long double 
__x, int *__exp ) {
+  return (long double) frexp((double)__x, __exp );
+}
+
+long double pain(void)
+{
+long double f = 123.45;
+int i;
+long double f2 = frexpl(f, &i);
+return f2;
+}
+
+// CHECK-NOT: define{{.*}}@frexpl(
+// CHECK: define dso_local double @pain
+// CHECK:[[CALL_I:%.*]] = call double @frexp(double noundef [[TMP2]], ptr 
noundef [[TMP1]]) #[[ATTR3:[0-9]+]]
+// CHECK: declare dso_local double @frexp(double noundef, ptr noundef)
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5189,9 +5189,10 @@
   if (D->hasAttr())
 return llvm::GlobalVariable::WeakAnyLinkage;
 
-  if (const auto *FD = D->getAsFunction())
+  if (const auto *FD = D->getAsFunction()) {
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
   return llvm::GlobalVariable::LinkOnceAnyLinkage;
+  }
 
   // We are guaranteed to have a strong definition somewhere else,
   // so we can use available_externally linkage.
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11535,7 +11535,7 @@
 
 static GVALinkage basicGVALinkageForFunction(const ASTContext &Context,
  const FunctionDecl *FD) {
-  if (!FD->isExternallyVisible())
+  if (!FD->isExternallyVisible() || FD->isInlineBuiltinDeclaration())
 return GVA_Internal;
 
   // Non-user-provided functions get emitted as weak definitions with every


Index: clang/test/CodeGen/inline-builtin-comdat.c
===
--- /dev/null
+++ clang/test/CodeGen/inline-builtin-comdat.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-windows -S -emit-llvm %s -o - | FileCheck %s
+// Make sure we don't generate definition for Inline Builtins on windows
+
+double __cdecl frexp( double _X, int* _Y);
+inline __attribute__((always_inline))  long double __cdecl frexpl( long double __x, int *__exp ) {
+  return (long double) frexp((double)__x, __exp );
+}
+
+long double pain(void)
+{
+long double f = 123.45;
+int i;
+long double f2 = frexpl(f, &i);
+return f2;
+}
+
+// CHECK-NOT: define{{.*}}@frexpl(
+// CHECK: define dso_local double @pain
+// CHECK:[[CALL_I:%.*]] = call double @frexp(double noundef [[TMP2]], ptr noundef [[TMP1]]) #[[ATTR3:[0-9]+]]
+// CHECK: declare dso_local double @frexp(double noundef, ptr noundef)
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5189,9 +5189,10 @@
   if (D->hasAttr())
 return llvm::GlobalVariable::WeakAnyLinkage;
 
-  if (const auto *FD = D->getAsFunction())
+  if (const auto *FD = D->getAsFunction()) {
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
   return llvm::GlobalVariable::LinkOnceAnyLinkage;
+  }
 
   // We are guaranteed to have a strong definition somewhere else,
   // so we can use available_externally linkage.
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11535,7 +11535,7 @@
 
 static GVALinkage basicGVALinkageForFunction(const ASTContext &Context,
  const FunctionDecl *FD) {
-  if (!FD->isExternallyVisible())
+  if (!FD->isExternallyVisible() || FD->isInlineBuiltinDeclaration())
 return GVA_Internal;
 
   // Non-user-provided functions get emitted as weak definitions with every
___
cfe-commits mailing li