[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> I'd like to also see a testcase for the situation where we trigger the 
> emission of a declaration with an available_externally definition and then 
> find we need to promote it to a "full" definition:

Added!




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+if (InitExpr) {
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  GV->setInitializer(EmitConstantInit(*InitDecl));
+}

rsmith wrote:
> mehdi_amini wrote:
> > rsmith wrote:
> > > In order for this transformation to be correct, you need to know that the 
> > > variable has static initialization, which means that it needs to formally 
> > > have a constant initializer. You can use `D->isInitKnownICE()` to check 
> > > this.
> > Done! But couldn't find how to express it as a test-case though.
> You'd need to use an initializer that we can constant-fold, such as:
> 
> ```
> struct A {
>   static const int n;
> };
> bool check() {
>   assert(A::n == 0 && "already initialized!");
>   return true;
> }
> const int A::n = (check() || true) ? 1 : 2;
> ```
But this wouldn't be an "available_externally". We need the initializer in the 
class definition.

I tried multiple variant but couldn't get one that compile:

```
struct B {
  static bool check() {
if (B::n == 0) return false;
return true;
  }
  // error: in-class initializer for static data member is not a constant 
expression
  static const int n = (B::check() || true) ? 1 : 2;
};
```

```
struct B {
  static constexpr bool check() {
// "error: constexpr function never produces a constant expression"
return  (B::n == 0 || true) ? false : true;
  }
//"note: initializer of 'n' is not a constant expression"
  static constexpr int n = (B::check() || true) ? 1 : 2;
};
```






Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2445
+!D->hasDefinition() && D->hasInit() &&
+/* C++17 static constexpr are inlined */ !D->isInline() &&
+!D->hasAttr() && D->isInitKnownICE()) {

rsmith wrote:
> Do we need to special-case this? Such declarations are definitions.
We don't! Simplified after rebasing and adapting after John's changes.


Repository:
  rL LLVM

https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL311857: Emit static constexpr member as available_externally 
definition (authored by mehdi_amini).

Changed prior to commit:
  https://reviews.llvm.org/D34992?vs=109694=112836#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34992

Files:
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp


Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -2437,6 +2437,28 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (Context.getLangOpts().CPlusPlus && GV->hasExternalLinkage() &&
+D->getType().isConstQualified() && !GV->hasInitializer() &&
+!D->hasDefinition() && D->hasInit() && !D->hasAttr()) {
+  const auto *Record =
+  Context.getBaseElementType(D->getType())->getAsCXXRecordDecl();
+  bool HasMutableFields = Record && Record->hasMutableFields();
+  if (!HasMutableFields) {
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
+  GV->setConstant(true);
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  ConstantEmitter emitter(*this);
+  GV->setInitializer(emitter.tryEmitForInitializer(*InitDecl));
+  emitter.finalize(GV);
+}
+  }
+}
   }
 
   auto ExpectedAS =
Index: cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
+++ cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct A {
+  static const int Foo = 123;
+};
+// CHECK: @_ZN1A3FooE = constant i32 123, align 4
+const int *p = ::Foo; // emit available_externally
+const int A::Foo;   // convert to full definition
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant 
i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 
43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant 
%struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant 
%struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global 
%struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = linkonce_odr 
global %struct.MutableBar { i32 45 },
+  static constexpr MutableBar ConstexprStaticMutableStructMember = {45};
+};
+// CHECK: @_ZL15ConstStaticexpr = internal constant i32 46,
+static constexpr int ConstStaticexpr = 46;
+// CHECK: @_ZL9ConstExpr = internal constant i32 46, align 4
+static const int ConstExpr = 46;
+
+// CHECK: @_ZL21ConstexprStaticStruct = internal constant %struct.Bar { i32 47 
},
+static constexpr Bar ConstexprStaticStruct = {47};
+
+// CHECK: @_ZL28ConstexprStaticMutableStruct = internal global 
%struct.MutableBar { i32 48 },
+static constexpr MutableBar ConstexprStaticMutableStruct = {48};
+
+void use(const int &);
+void foo() {
+  use(Foo::ConstexprStaticMember);
+  use(Foo::ConstStaticMember);
+  use(Foo::ConstStaticStructMember.b);
+  use(Foo::ConstexprStaticMutableStructMember.b);
+  use(ConstStaticexpr);
+  use(ConstExpr);
+  use(ConstexprStaticStruct.b);
+  use(ConstexprStaticMutableStruct.b);
+}


Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -2437,6 +2437,28 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (Context.getLangOpts().CPlusPlus && 

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

I'd like to also see a testcase for the situation where we trigger the emission 
of a declaration with an `available_externally` definition and then find we 
need to promote it to a "full" definition:

  struct A {
static const int Foo = 123;
  };
  int *p = ::Foo; // emit available_externally
  const int A::Foo; // convert to full definition




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+if (InitExpr) {
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  GV->setInitializer(EmitConstantInit(*InitDecl));
+}

mehdi_amini wrote:
> rsmith wrote:
> > In order for this transformation to be correct, you need to know that the 
> > variable has static initialization, which means that it needs to formally 
> > have a constant initializer. You can use `D->isInitKnownICE()` to check 
> > this.
> Done! But couldn't find how to express it as a test-case though.
You'd need to use an initializer that we can constant-fold, such as:

```
struct A {
  static const int n;
};
bool check() {
  assert(A::n == 0 && "already initialized!");
  return true;
}
const int A::n = (check() || true) ? 1 : 2;
```



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2445
+!D->hasDefinition() && D->hasInit() &&
+/* C++17 static constexpr are inlined */ !D->isInline() &&
+!D->hasAttr() && D->isInitKnownICE()) {

Do we need to special-case this? Such declarations are definitions.


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

FYI, this doesn't compiler after John's constant emitter refactoring (r310964)


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked an inline comment as done.
mehdi_amini added a comment.

Bi-weekly ping! (@rsmith)


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked 6 inline comments as done.
mehdi_amini added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2293
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify 
that
+/// the object is not written to during its construction.

rsmith wrote:
> This comment change suggests that when `ExcludeCtor` is true, a trivial 
> constructor is considered. That's the opposite of what this change does -- 
> namely, a trivial constructor is not considered regardless of the value of 
> `ExcludeCtor`.
Yep your right. I removed all the changes here.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2437-2445
+  bool HasMutableField = false;
+  if (Context.getLangOpts().CPlusPlus) {
+if (const CXXRecordDecl *Record =
+Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+  HasMutableField = Record->hasMutableFields();
+  }
+

rsmith wrote:
> This duplicates much of the work done by `isTypeConstant`.
Indeed, thanks for pointing this, this simplifies a lot!



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+if (InitExpr) {
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  GV->setInitializer(EmitConstantInit(*InitDecl));
+}

rsmith wrote:
> In order for this transformation to be correct, you need to know that the 
> variable has static initialization, which means that it needs to formally 
> have a constant initializer. You can use `D->isInitKnownICE()` to check this.
Done! But couldn't find how to express it as a test-case though.


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini updated this revision to Diff 109694.
mehdi_amini marked 12 inline comments as done.
mehdi_amini added a comment.

Address Richard's comment


https://reviews.llvm.org/D34992

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp


Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant 
i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 
43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant 
%struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant 
%struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global 
%struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = linkonce_odr 
global %struct.MutableBar { i32 45 },
+  static constexpr MutableBar ConstexprStaticMutableStructMember = {45};
+};
+// CHECK: @_ZL15ConstStaticexpr = internal constant i32 46,
+static constexpr int ConstStaticexpr = 46;
+// CHECK: @_ZL9ConstExpr = internal constant i32 46, align 4
+static const int ConstExpr = 46;
+
+// CHECK: @_ZL21ConstexprStaticStruct = internal constant %struct.Bar { i32 47 
},
+static constexpr Bar ConstexprStaticStruct = {47};
+
+// CHECK: @_ZL28ConstexprStaticMutableStruct = internal global 
%struct.MutableBar { i32 48 },
+static constexpr MutableBar ConstexprStaticMutableStruct = {48};
+
+void use(const int &);
+void foo() {
+  use(Foo::ConstexprStaticMember);
+  use(Foo::ConstStaticMember);
+  use(Foo::ConstStaticStructMember.b);
+  use(Foo::ConstexprStaticMutableStructMember.b);
+  use(ConstStaticexpr);
+  use(ConstExpr);
+  use(ConstexprStaticStruct.b);
+  use(ConstexprStaticMutableStruct.b);
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2436,6 +2436,22 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose its value to the
+// optimizer.
+if (GV->hasExternalLinkage() && isTypeConstant(D->getType(), true) &&
+!D->hasDefinition() && D->hasInit() &&
+/* C++17 static constexpr are inlined */ !D->isInline() &&
+!D->hasAttr() && D->isInitKnownICE()) {
+  const VarDecl *InitDecl;
+  const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+  if (InitExpr) {
+GV->setConstant(true);
+GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+GV->setInitializer(EmitConstantInit(*InitDecl));
+  }
+}
   }
 
   auto ExpectedAS =


Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant %struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant %struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global %struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = 

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2293
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify 
that
+/// the object is not written to during its construction.

This comment change suggests that when `ExcludeCtor` is true, a trivial 
constructor is considered. That's the opposite of what this change does -- 
namely, a trivial constructor is not considered regardless of the value of 
`ExcludeCtor`.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2302
+Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
+  return (ExcludeCtor || Record->hasTrivialDefaultConstructor()) &&
+ !Record->hasMutableFields() && Record->hasTrivialDestructor();

This change looks wrong: you don't know that the default constructor would be 
used to initialize the object in question, so whether the default constructor 
is trivial seems like it should have no bearing on the result.

I think you can change the code below that uses `GV->isConstant()` to call 
`isTypeConstant` instead, and remove the need for any changes here.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2433
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.

Typo "it's" -> "its"



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2435
+// optimizer.
+if (GV->hasExternalLinkage() && GV->isConstant() && !GV->hasInitializer() 
&&
+!D->hasDefinition() && D->hasInit()) {

Instead of `GV->isConstant()`, it would make more sense to use 
`isTypeConstant(D->getType(), true)` here.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2437-2445
+  bool HasMutableField = false;
+  if (Context.getLangOpts().CPlusPlus) {
+if (const CXXRecordDecl *Record =
+Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+  HasMutableField = Record->hasMutableFields();
+  }
+

This duplicates much of the work done by `isTypeConstant`.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+if (InitExpr) {
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+  GV->setInitializer(EmitConstantInit(*InitDecl));
+}

In order for this transformation to be correct, you need to know that the 
variable has static initialization, which means that it needs to formally have 
a constant initializer. You can use `D->isInitKnownICE()` to check this.


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Ping again @rsmith (or anyone else)


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Weekly ping! (@rsmith)


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

@rsmith: post-C++-commitee-meeting ping ;)


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini updated this revision to Diff 105787.
mehdi_amini added a comment.

Fix issues around mutable fields and regression on "internal", add more testing.


https://reviews.llvm.org/D34992

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant %struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant %struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global %struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = linkonce_odr global %struct.MutableBar { i32 45 },
+  static constexpr MutableBar ConstexprStaticMutableStructMember = {45};
+};
+// CHECK: @_ZL15ConstStaticexpr = internal constant i32 46,
+static constexpr int ConstStaticexpr = 46;
+// CHECK: @_ZL9ConstExpr = internal constant i32 46, align 4
+static const int ConstExpr = 46;
+
+// CHECK: @_ZL21ConstexprStaticStruct = internal constant %struct.Bar { i32 47 },
+static constexpr Bar ConstexprStaticStruct = {47};
+
+// CHECK: @_ZL28ConstexprStaticMutableStruct = internal global %struct.MutableBar { i32 48 },
+static constexpr MutableBar ConstexprStaticMutableStruct = {48};
+
+void use(const int &);
+void foo() {
+  use(Foo::ConstexprStaticMember);
+  use(Foo::ConstStaticMember);
+  use(Foo::ConstStaticStructMember.b);
+  use(Foo::ConstexprStaticMutableStructMember.b);
+  use(ConstStaticexpr);
+  use(ConstExpr);
+  use(ConstexprStaticStruct.b);
+  use(ConstexprStaticMutableStruct.b);
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2290,17 +2290,17 @@
 /// as a constant.
 ///
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify that
+/// the object is not written to during its construction.
 bool CodeGenModule::isTypeConstant(QualType Ty, bool ExcludeCtor) {
   if (!Ty.isConstant(Context) && !Ty->isReferenceType())
 return false;
 
   if (Context.getLangOpts().CPlusPlus) {
-if (const CXXRecordDecl *Record
-  = Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
-  return ExcludeCtor && !Record->hasMutableFields() &&
- Record->hasTrivialDestructor();
+if (const CXXRecordDecl *Record =
+Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
+  return (ExcludeCtor || Record->hasTrivialDefaultConstructor()) &&
+ !Record->hasMutableFields() && Record->hasTrivialDestructor();
   }
 
   return true;
@@ -2372,7 +2372,7 @@
 
   auto *GV = new llvm::GlobalVariable(
   getModule(), Ty->getElementType(), false,
-  llvm::GlobalValue::ExternalLinkage, nullptr, MangledName, nullptr,
+  llvm::GlobalVariable::ExternalLinkage, nullptr, MangledName, nullptr,
   llvm::GlobalVariable::NotThreadLocal, TargetAddrSpace);
 
   // If we already created a global with the same mangled name (but different
@@ -2428,6 +2428,29 @@
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (GV->hasExternalLinkage() && GV->isConstant() && !GV->hasInitializer() &&
+!D->hasDefinition() && D->hasInit()) {
+  bool HasMutableField = false;
+  if (Context.getLangOpts().CPlusPlus) {
+if (const CXXRecordDecl *Record =
+Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+  HasMutableField = Record->hasMutableFields();
+  }
+
+  

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:15
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+

mehdi_amini wrote:
> rsmith wrote:
> > mehdi_amini wrote:
> > > Looks like I have a bug here, this should be an internal.
> > I would imagine that we skip promotion of declaration to definition in this 
> > case if we already have a definition.
> > 
> > To that end, please add a testcase like this:
> > 
> > ```
> > struct Bar {
> >   static constexpr int BAZ = 42;
> > };
> > auto *use = ::BAZ;
> > const int Bar::BAZ;
> > ```
> > 
> > ... to make sure that we convert the definition of `Bar::BAZ` from 
> > `available_externally` to a strong definition (I think we should end up 
> > with `weak_odr` here).
> `weak_odr` in C++17 because it is an inline variable, but I expect a strong 
> definition in c++11.
> I'll add this, this is a good test-case!
Well, `weak_odr` is a kind of strong definition :)

Ah, I'd intended to change from emitting this as a strong definition to 
emitting it as `weak_odr` even in C++11 (for better forward-compatibility with 
C++17), but it looks like I never did so. So yes, we'd expect an 
ExternalLinkage global here (for now) in C++11.


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2374
+  llvm::Constant *Init = nullptr;
+  if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr()) 
{
+const VarDecl *InitDecl;

rsmith wrote:
> Applying this for only `constexpr` variables seems unnecessarily 
> conservative; it seems we could equally do this for any variable that is 
> `const` and has no `mutable` fields (though we'd need to check for 
> `EmitConstantInit` failing in the general case).
OK I will improve, and include a test with a struct with a mutable field.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:1
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s
+

rsmith wrote:
> Please also test what happens in C++1z mode, particularly as static constexpr 
> data members are implicitly `inline` there.
This is already covered by ```clang/test/CodeGenCXX/cxx1z-inline-variables.cpp 
``` (I actually broke this test during development because I didn't know about 
inline variable). But I can also add coverage for it here if you'd like.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:15
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+

rsmith wrote:
> mehdi_amini wrote:
> > Looks like I have a bug here, this should be an internal.
> I would imagine that we skip promotion of declaration to definition in this 
> case if we already have a definition.
> 
> To that end, please add a testcase like this:
> 
> ```
> struct Bar {
>   static constexpr int BAZ = 42;
> };
> auto *use = ::BAZ;
> const int Bar::BAZ;
> ```
> 
> ... to make sure that we convert the definition of `Bar::BAZ` from 
> `available_externally` to a strong definition (I think we should end up with 
> `weak_odr` here).
`weak_odr` in C++17 because it is an inline variable, but I expect a strong 
definition in c++11.
I'll add this, this is a good test-case!


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

We've had problems in the past with speculative emission of values like this 
resulting in us producing invalid symbol references. (Two cases I remember: an 
`available_externally` symbol references a symbol that should be emitted as 
`linkonce_odr` but which is not emitted in this TU, and an 
`available_externally` symbol references a symbol with `hidden` visibility that 
is actually defined in a different DSO. In both cases, if the value of the 
`available_externally` symbol is actually used, you end up with a program with 
an invalid symbol reference.)

I don't immediately see any ways that this change would be susceptible to such 
a problem, so perhaps our best bet is to try it and see if it actually breaks 
anything.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2374
+  llvm::Constant *Init = nullptr;
+  if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr()) 
{
+const VarDecl *InitDecl;

Applying this for only `constexpr` variables seems unnecessarily conservative; 
it seems we could equally do this for any variable that is `const` and has no 
`mutable` fields (though we'd need to check for `EmitConstantInit` failing in 
the general case).



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {

ahatanak wrote:
> Does getAnyInitializer ever return a null pointer here when D is a c++11 
> constexpr?
Only in error cases, which shouldn't get this far.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:1
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s
+

Please also test what happens in C++1z mode, particularly as static constexpr 
data members are implicitly `inline` there.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:15
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+

mehdi_amini wrote:
> Looks like I have a bug here, this should be an internal.
I would imagine that we skip promotion of declaration to definition in this 
case if we already have a definition.

To that end, please add a testcase like this:

```
struct Bar {
  static constexpr int BAZ = 42;
};
auto *use = ::BAZ;
const int Bar::BAZ;
```

... to make sure that we convert the definition of `Bar::BAZ` from 
`available_externally` to a strong definition (I think we should end up with 
`weak_odr` here).


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {

ahatanak wrote:
> Does getAnyInitializer ever return a null pointer here when D is a c++11 
> constexpr?
That's a good question, I wouldn't expect so. I can try adding an assertion 
instead.
I guess @rsmith could confirm as well.


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2376
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {

Does getAnyInitializer ever return a null pointer here when D is a c++11 
constexpr?


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:12
+// CHECK: @_ZN3Foo4BAR2E = external constant i32,
+  static const int BAR2 = 43; 
+};

Note: I'm not sure if the standard allows us to assume that we can fold the 
value for BAR2 without seeing the definition here? 
(so we could be even more aggressive)



Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:15
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+

Looks like I have a bug here, this should be an internal.


https://reviews.llvm.org/D34992



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

Re: [PATCH] D34992: Emit static constexpr member as available_externally definition

[David Majnemer via cfe-commits]

On Wed, Jul 5, 2017 at 12:22 PM Mehdi AMINI  wrote:

> The LLVM verifier is complaining that dllimport have to be external
> linkage and isn't happy with available_externally, is the verifier wrong?
>

IMO, yes. I imagine that it is fine with dllimport available_externally
functions already.


> 2017-07-05 9:12 GMT-07:00 David Majnemer :
>
>> I don't think you need the dllimport restriction.
>>
>> On Wed, Jul 5, 2017 at 12:05 PM Alex Lorenz via Phabricator via
>> cfe-commits  wrote:
>>
>>> arphaman added a comment.
>>>
>>> Does this apply to all constexpr global variables? It could potentially
>>> fix https://bugs.llvm.org/show_bug.cgi?id=31860 .
>>>
>>>
>>> https://reviews.llvm.org/D34992
>>>
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via cfe-commits]

The LLVM verifier is complaining that dllimport have to be external linkage
and isn't happy with available_externally, is the verifier wrong?

2017-07-05 9:12 GMT-07:00 David Majnemer :

> I don't think you need the dllimport restriction.
>
> On Wed, Jul 5, 2017 at 12:05 PM Alex Lorenz via Phabricator via
> cfe-commits  wrote:
>
>> arphaman added a comment.
>>
>> Does this apply to all constexpr global variables? It could potentially
>> fix https://bugs.llvm.org/show_bug.cgi?id=31860 .
>>
>>
>> https://reviews.llvm.org/D34992
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34992: Emit static constexpr member as available_externally definition

[David Majnemer via cfe-commits]

I don't think you need the dllimport restriction.

On Wed, Jul 5, 2017 at 12:05 PM Alex Lorenz via Phabricator via cfe-commits
 wrote:

> arphaman added a comment.
>
> Does this apply to all constexpr global variables? It could potentially
> fix https://bugs.llvm.org/show_bug.cgi?id=31860 .
>
>
> https://reviews.llvm.org/D34992
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Does this apply to all constexpr global variables? It could potentially fix 
https://bugs.llvm.org/show_bug.cgi?id=31860 .


https://reviews.llvm.org/D34992



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

[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini created this revision.

This enables better optimization, I don't if it is legal c++11 though.


https://reviews.llvm.org/D34992

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp


Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s
+
+template
+int min(const T , const T ) {
+   return a > b ? b : a;
+}
+
+struct Foo {
+// CHECK: @_ZN3Foo3BARE = available_externally constant i32 42,
+  static constexpr int BAR = 42; 
+// CHECK: @_ZN3Foo4BAR2E = external constant i32,
+  static const int BAR2 = 43; 
+};
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+
+int foo(int v) {
+   return min(min(Foo::BAR, Foo::BAR2), BAR3);
+}
+
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2368,11 +2368,22 @@
   return llvm::ConstantExpr::getBitCast(Entry, Ty);
   }
 
+  auto Linkage = llvm::GlobalValue::ExternalLinkage;
+
+  llvm::Constant *Init = nullptr;
+  if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr()) 
{
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
+  Init = EmitConstantInit(*InitDecl);
+  Linkage = llvm::GlobalValue::AvailableExternallyLinkage;
+}
+  }
+
   unsigned AddrSpace = GetGlobalVarAddressSpace(D, Ty->getAddressSpace());
   auto *GV = new llvm::GlobalVariable(
-  getModule(), Ty->getElementType(), false,
-  llvm::GlobalValue::ExternalLinkage, nullptr, MangledName, nullptr,
-  llvm::GlobalVariable::NotThreadLocal, AddrSpace);
+  getModule(), Ty->getElementType(), false, Linkage, Init, MangledName,
+  nullptr, llvm::GlobalVariable::NotThreadLocal, AddrSpace);
 
   // If we already created a global with the same mangled name (but different
   // type) before, take its name and remove it from its parent.


Index: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s
+
+template
+int min(const T , const T ) {
+	return a > b ? b : a;
+}
+
+struct Foo {
+// CHECK: @_ZN3Foo3BARE = available_externally constant i32 42,
+  static constexpr int BAR = 42; 
+// CHECK: @_ZN3Foo4BAR2E = external constant i32,
+  static const int BAR2 = 43; 
+};
+// CHECK: @_ZL4BAR3 = available_externally constant i32 44,
+static constexpr int BAR3 = 44;
+
+int foo(int v) {
+	return min(min(Foo::BAR, Foo::BAR2), BAR3);
+}
+
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2368,11 +2368,22 @@
   return llvm::ConstantExpr::getBitCast(Entry, Ty);
   }
 
+  auto Linkage = llvm::GlobalValue::ExternalLinkage;
+
+  llvm::Constant *Init = nullptr;
+  if (D && D->isConstexpr() && !D->isInline() && !D->hasAttr()) {
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
+  Init = EmitConstantInit(*InitDecl);
+  Linkage = llvm::GlobalValue::AvailableExternallyLinkage;
+}
+  }
+
   unsigned AddrSpace = GetGlobalVarAddressSpace(D, Ty->getAddressSpace());
   auto *GV = new llvm::GlobalVariable(
-  getModule(), Ty->getElementType(), false,
-  llvm::GlobalValue::ExternalLinkage, nullptr, MangledName, nullptr,
-  llvm::GlobalVariable::NotThreadLocal, AddrSpace);
+  getModule(), Ty->getElementType(), false, Linkage, Init, MangledName,
+  nullptr, llvm::GlobalVariable::NotThreadLocal, AddrSpace);
 
   // If we already created a global with the same mangled name (but different
   // type) before, take its name and remove it from its parent.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits