[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[Hongtao Yu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG633ca3ff2f8f: [UniqueLinkageName] Use exsiting GlobalDecl 
object instead of reconstructing… (authored by hoy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,12 +42,26 @@
   return mver();
 }
 
+namespace {
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+}
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AC1Ev
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AD1Ev
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
@@ -57,6 +71,8 @@
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]() 
#[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.[[MODHASH]]
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.[[MODHASH]].resolver()
+// UNIQUE: define internal void 
@_ZN12_GLOBAL__N_11AC1Ev.__uniq.68358509610070717889884130747296293671
+// UNIQUE: define internal void 
@_ZN12_GLOBAL__N_11AD1Ev.__uniq.68358509610070717889884130747296293671
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]]()
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { 
{{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2174,7 +2174,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,12 +42,26 @@
   return mver();
 }
 
+namespace {
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+}
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AC1Ev
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AD1Ev
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
@@ -57,6 +71,8 @@
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]() #[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.[[MODHASH]]
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.[[MODHASH]].resolver()
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AC1Ev.__uniq.68358509610070717889884130747296293671
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AD1Ev.__uniq.68358509610070717889884130747296293671
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]]()
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { {{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2174,7 +2174,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[David Blaikie via Phabricator via cfe-commits]

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

Looks alright, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 355012.
hoy added a comment.
Herald added a subscriber: modimo.

Added tests for static ctor/dtor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,12 +42,26 @@
   return mver();
 }
 
+namespace {
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+}
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AC1Ev
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AD1Ev
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
@@ -57,6 +71,8 @@
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]() 
#[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.[[MODHASH]]
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.[[MODHASH]].resolver()
+// UNIQUE: define internal void 
@_ZN12_GLOBAL__N_11AC1Ev.__uniq.68358509610070717889884130747296293671
+// UNIQUE: define internal void 
@_ZN12_GLOBAL__N_11AD1Ev.__uniq.68358509610070717889884130747296293671
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]]()
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { 
{{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2174,7 +2174,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,12 +42,26 @@
   return mver();
 }
 
+namespace {
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+}
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AC1Ev
+// PLAIN: define internal void @_ZN12_GLOBAL__N_11AD1Ev
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
@@ -57,6 +71,8 @@
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]() #[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.[[MODHASH]]
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.[[MODHASH]].resolver()
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AC1Ev.__uniq.68358509610070717889884130747296293671
+// UNIQUE: define internal void @_ZN12_GLOBAL__N_11AD1Ev.__uniq.68358509610070717889884130747296293671
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]]()
 // UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { {{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2174,7 +2174,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }
___
cfe-commits mailing list

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D102356#2761010 , @hoy wrote:

> In D102356#2760973 , @dblaikie 
> wrote:
>
>> In D102356#2758371 , @hoy wrote:
>>
>>> In D102356#2758179 , @dblaikie 
>>> wrote:
>>>
 This was previously crashing, I guess? Testing should validate the 
 behavior beyond the crash, though - (presumably there's some more specific 
 behavior than "does not crash" that wasn't being tested for before - that 
 certain names are mangled appropriately or what-have-you)
>>>
>>> Yes, an assert was triggered related to c++ constructors/destructors while 
>>> it's not now. Regarding the behavior, c++ constructors/destructors are not 
>>> static, so I don't expect a behavior change.
>>
>> ctors/dtors can have internal linkage, if the type has internal linkage (if 
>> it's in an anonymous namespace) - but in any case, my point was that there's 
>> some specific behavior you're expecting, even if that behavior is "does not 
>> get this attribute" - previously no code tested that with this feature 
>> enabled the ctor wouldn't get the attribute (because it couldn't've tested 
>> that, because what it did was crash) - so testing that would be good.
>>
>> But testing the attribute does work on the ctor of a type in an anonymous 
>> namespace would be suitable too.
>
> Good to know that ctors/dtors can have internal linkage. How do you get that? 
> The C++ standard doesn't allow ctors to be static.

Placing the type within an anonymous namespace should do the trick, something 
like this: https://godbolt.org/z/8YMWhbWM8


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D102356#2760973 , @dblaikie wrote:

> In D102356#2758371 , @hoy wrote:
>
>> In D102356#2758179 , @dblaikie 
>> wrote:
>>
>>> This was previously crashing, I guess? Testing should validate the behavior 
>>> beyond the crash, though - (presumably there's some more specific behavior 
>>> than "does not crash" that wasn't being tested for before - that certain 
>>> names are mangled appropriately or what-have-you)
>>
>> Yes, an assert was triggered related to c++ constructors/destructors while 
>> it's not now. Regarding the behavior, c++ constructors/destructors are not 
>> static, so I don't expect a behavior change.
>
> ctors/dtors can have internal linkage, if the type has internal linkage (if 
> it's in an anonymous namespace) - but in any case, my point was that there's 
> some specific behavior you're expecting, even if that behavior is "does not 
> get this attribute" - previously no code tested that with this feature 
> enabled the ctor wouldn't get the attribute (because it couldn't've tested 
> that, because what it did was crash) - so testing that would be good.
>
> But testing the attribute does work on the ctor of a type in an anonymous 
> namespace would be suitable too.

Good to know that ctors/dtors can have internal linkage. How do you get that? 
The C++ standard doesn't allow ctors to be static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D102356#2758371 , @hoy wrote:

> In D102356#2758179 , @dblaikie 
> wrote:
>
>> This was previously crashing, I guess? Testing should validate the behavior 
>> beyond the crash, though - (presumably there's some more specific behavior 
>> than "does not crash" that wasn't being tested for before - that certain 
>> names are mangled appropriately or what-have-you)
>
> Yes, an assert was triggered related to c++ constructors/destructors while 
> it's not now. Regarding the behavior, c++ constructors/destructors are not 
> static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if 
it's in an anonymous namespace) - but in any case, my point was that there's 
some specific behavior you're expecting, even if that behavior is "does not get 
this attribute" - previously no code tested that with this feature enabled the 
ctor wouldn't get the attribute (because it couldn't've tested that, because 
what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous 
namespace would be suitable too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D102356#2758179 , @dblaikie wrote:

> This was previously crashing, I guess? Testing should validate the behavior 
> beyond the crash, though - (presumably there's some more specific behavior 
> than "does not crash" that wasn't being tested for before - that certain 
> names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's 
not now. Regarding the behavior, c++ constructors/destructors are not static, 
so I don't expect a behavior change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

This was previously crashing, I guess? Testing should validate the behavior 
beyond the crash, though - (presumably there's some more specific behavior than 
"does not crash" that wasn't being tested for before - that certain names are 
mangled appropriately or what-have-you)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102356

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

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

[Hongtao Yu via Phabricator via cfe-commits]

hoy created this revision.
Herald added a subscriber: wenlei.
hoy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

C++ constructors/destructors need to go through a different constructor to 
construct a GlobalDecl object in order to retrieve their linkage type. This 
causes an assert failure in the default constructor of GlobalDecl. I'm chaning 
it to using the exsiting GlobalDecl object.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102356

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,6 +42,16 @@
   return mver();
 }
 
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2171,7 +2171,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }


Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -42,6 +42,16 @@
   return mver();
 }
 
+class A {
+public:
+  A() {}
+  ~A() {}
+};
+
+void test() {
+  A a;
+}
+
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2171,7 +2171,8 @@
   // functions with -funique-internal-linkage-names.
   if (TargetDecl && CodeGenOpts.UniqueInternalLinkageNames) {
 if (auto *Fn = dyn_cast(TargetDecl)) {
-  if (this->getFunctionLinkage(Fn) == llvm::GlobalValue::InternalLinkage)
+  if (this->getFunctionLinkage(CalleeInfo.getCalleeDecl()) ==
+  llvm::GlobalValue::InternalLinkage)
 FuncAttrs.addAttribute("sample-profile-suffix-elision-policy",
"selected");
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits