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

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:

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

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

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

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"

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

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

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

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 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

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

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:

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

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

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

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 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 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 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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