[PATCH] D135926: [clang] Fix crash with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

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

LGTM, thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135926

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

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram accepted this revision.
tmsriram added inline comments.



Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:589
+
+if (SampleProfileHasFUnique) {
+  // If profile also uses funqiue, nothing to do here.

Maybe rewrite this slightly as:

//  If Sample Profile and Instrumented Profile do not agree on symbol 
uniqification.
if (SampleProfileHasFunique != ProfileHasFUnique) {
   if (ProfileHasFUnique) {
  // trim
   } else {
  // Build Map
   }
}


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

https://reviews.llvm.org/D132600

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

[PATCH] D105226: [Clang] allow overriding -fbasic-block-sections

[Sriraman Tallam via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105226

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

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:18
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{

Nice test, I didnt know you could do this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98799

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

[PATCH] D98392: Disable Unique Internal Linkage Names for internal global vars

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcdb42a4cc423: Disable unique linkage suffixes ifor global 
vars until demanglers can be fixed. (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D98392?vs=330130=330136#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98392

Files:
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.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
@@ -51,12 +51,12 @@
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // PLAIN-NOT: "sample-profile-suffix-elision-policy"
-// UNIQUE: @_ZL4glob.__uniq.{{[0-9]+}} = internal global
-// UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9]+}} = internal global
-// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9]+}} = internal global
-// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}() #[[#ATTR:]] {
-// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9]+}}
-// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.__uniq.{{[0-9]+}}.resolver()
-// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}.sse4.2
+// UNIQUE: @_ZL4glob = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE = internal global
+// 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 i32 @_ZL4mverv.[[MODHASH]]()
+// UNIQUE: define internal i32 @_ZL4mverv.[[MODHASH]].sse4.2
 // UNIQUE: attributes #[[#ATTR]] = { {{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.cpp
@@ -46,15 +46,15 @@
 // PLAIN-DAG: distinct !DIGlobalVariable(name: "glob_zip", linkageName: "_ZL8glob_zip"{{.*}})
 // PLAIN-DAG: distinct !DISubprogram(name: "zip", linkageName: "_ZL3zipv"{{.*}})
 
-// UNIQUE-DAG: @_ZL8glob_foo.[[MODHASH:__uniq\.[0-9]+]] = internal global i32
-// UNIQUE-DAG: define internal i32 @_ZL3foov.[[MODHASH]]()
-// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_foo", linkageName: "_ZL8glob_foo.[[MODHASH]]"{{.*}})
+// UNIQUE-DAG: @_ZL8glob_foo = internal global i32
+// UNIQUE-DAG: define internal i32 @_ZL3foov.[[MODHASH:__uniq\.[0-9]+]]()
+// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_foo", linkageName: "_ZL8glob_foo"{{.*}})
 // UNIQUE-DAG: distinct !DISubprogram(name: "foo", linkageName: "_ZL3foov.[[MODHASH]]"{{.*}})
-// UNIQUE-DAG: @_ZN12_GLOBAL__N_18glob_barE.[[MODHASH]] = internal global i32
+// UNIQUE-DAG: @_ZN12_GLOBAL__N_18glob_barE = internal global i32
 // UNIQUE-DAG: define internal i32 @_ZN12_GLOBAL__N_13barEv.[[MODHASH]]()
-// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_bar", linkageName: "_ZN12_GLOBAL__N_18glob_barE.[[MODHASH]]"{{.*}})
+// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_bar", linkageName: "_ZN12_GLOBAL__N_18glob_barE"{{.*}})
 // UNIQUE-DAG: distinct !DISubprogram(name: "bar", linkageName: "_ZN12_GLOBAL__N_13barEv.[[MODHASH]]"{{.*}})
-// UNIQUE-DAG: @_ZL8glob_zip.[[MODHASH]] = internal global i32
+// UNIQUE-DAG: @_ZL8glob_zip = internal global i32
 // UNIQUE-DAG: define internal i32 @_ZL3zipv.[[MODHASH]]()
-// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_zip", linkageName: "_ZL8glob_zip.[[MODHASH]]"{{.*}})
+// UNIQUE-DAG: distinct !DIGlobalVariable(name: "glob_zip", linkageName: "_ZL8glob_zip"{{.*}})
 // UNIQUE-DAG: distinct !DISubprogram(name: "zip", linkageName: "_ZL3zipv.[[MODHASH]]"{{.*}})
Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
===
--- clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
@@ -22,7 +22,7 @@
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
 // PLAIN-NOT: linkageName:
 //
-// UNIQUE: @_ZL4glob.[[MODHASH:__uniq.[0-9]+]] = internal global i32
-// UNIQUE: define internal i32 @_ZL3foov.[[MODHASH]]()
-// UNIQUE: distinct !DIGlobalVariable(name: "glob", linkageName: 

[PATCH] D96109: Refactor implementation of -funique-internal-linkage-names.

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG78d0e91865f6: Refactor -funique-internal-linakge-names 
implementation. (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D96109?vs=325974=328635#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96109

Files:
  clang/include/clang/AST/Mangle.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.c
  clang/test/CodeGen/unique-internal-linkage-names-dwarf.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
@@ -1,10 +1,7 @@
 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
+// RUN: %clang_cc1 -triple x86_64 -x c++  -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
 static int foo() {
@@ -53,15 +50,13 @@
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// PLAIN-NOT: "sample-profile-suffix-elision-policy"
 // UNIQUE: @_ZL4glob.__uniq.{{[0-9]+}} = internal global
 // UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9]+}} = internal global
 // UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9]+}} = internal global
-// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
+// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}() #[[#ATTR:]] {
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9]+}}
-// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.__uniq.{{[0-9]+}}.resolver()
 // UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}
-// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
-// UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
-// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}
+// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}.sse4.2
+// UNIQUE: attributes #[[#ATTR]] = { {{.*}}"sample-profile-suffix-elision-policy"{{.*}} }
Index: clang/test/CodeGen/unique-internal-linkage-names-dwarf.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names-dwarf.cpp
@@ -0,0 +1,60 @@
+// This test checks if C++ functions with internal linkage names are mangled
+// and the module hash suffixes attached including emitting DW_AT_linkage_name.
+//
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -emit-llvm -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -funique-internal-linkage-names -emit-llvm -o - %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -emit-llvm -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o - %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob_foo = 5;
+static int foo(void) {
+  __builtin_printf("%p", _foo);
+  return glob_foo;
+}
+
+// Anonymous namespaces generate internal linkage symbols.
+namespace {
+  int glob_bar;
+  int bar() {
+return glob_bar;
+  }
+}
+
+extern "C" {
+  static int glob_zip;
+  static int zip(void) {
+return glob_zip;
+  }
+}
+
+void baz() {
+  foo();
+  bar();
+  zip();
+}
+
+// 

[PATCH] D94154: Unique Internal Linkage Name suffixes must be demangler friendly

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd8c6d24359f1: -funique-internal-linkage-names appends a hex 
md5hash suffix to the symbol name… (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94154

Files:
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll


Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
===
--- llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
@@ -10,5 +10,5 @@
   ret i32 0
 }
 
-; CHECK: @glob.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
-; CHECK: define internal i32 @foo.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d()
+; CHECK: @glob.__uniq.14209847432252523067699167782238157 = internal global
+; CHECK: define internal i32 
@foo.__uniq.14209847432252523067699167782238157()
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -27,9 +27,12 @@
   Md5.final(R);
   SmallString<32> Str;
   llvm::MD5::stringifyResult(R, Str);
+  // Convert MD5hash to Decimal. Demangler suffixes can either contain numbers
+  // or characters but not both.
+  APInt IntHash = APInt(128, Str.str(), 16);
   // Prepend "__uniq" before the hash for tools like profilers to understand 
that
   // this symbol is of internal linkage type.
-  std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
+  std::string ModuleNameHash = (Twine(".__uniq.") + Twine(IntHash.toString(10, 
false))).str();
   bool Changed = false;
 
   // Append the module hash to all internal linkage functions.
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
@@ -53,15 +53,15 @@
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
-// UNIQUE: @_ZL4glob.__uniq.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9a-f]+}} = internal global
-// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9a-f]+}}
+// UNIQUE: @_ZL4glob.__uniq.{{[0-9]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9]+}}
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
-// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}
+// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9]+}}()
 // UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
-// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9]+}}()
+// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9]+}}


Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
===
--- llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
@@ -10,5 +10,5 @@
   ret i32 0
 }
 
-; CHECK: @glob.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
-; CHECK: define internal i32 @foo.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d()
+; CHECK: @glob.__uniq.14209847432252523067699167782238157 = internal global
+; CHECK: define internal i32 @foo.__uniq.14209847432252523067699167782238157()
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -27,9 +27,12 @@

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2488387 , @dblaikie wrote:

> Seems alright to me - I think we've hashed out the deeper issues (missing 
> opportunity for C functions which could/should be addressed by moving the 
> implementation to the frontend, where those C functions can be mangled and 
> then use linkageName to give them the same AutoFDO opportunities as C++ 
> functions) here and elsewhere - but for what it is, the patch makes sense. 
> I'd probably say drop the flag - " check if rawLinkageName is set and only 
> set it when it is not null. " was implemented and seems that addressed the 
> debug info issue without an awkward tradeoff between AutoFDO fidelity and 
> debugging fidelity, so there doesn't seem to be a need to be able to 
> configure this.

Here is a suggestion for a plan forward. Let's get these patches along with 
D94154  in.  No correctness issues but a 
missed opportunity.  I will work with @rnk and @dblaikie and send out a patch 
where I move the uniqueification to clang?  That patch will also do linkage 
name for C functions with mangled name when uniqueification is needed. Does 
that sound reasonable?  As for timeline, I can do this in two weeks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

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

In D92633#2485811 , @MaskRay wrote:

> In D92633#2434768 , @tmsriram wrote:
>
>> In D92633#2434766 , @MaskRay wrote:
>>
>>> In D92633#2434714 , @tmsriram 
>>> wrote:
>>>
 Correct me if I am wrong, but I do see that this behavior is touched.  
 Line 10 in -fdirect-access-external-data.c :

 // RUN: %clang -### -c -target aarch64 %s -fpic 
 -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT

 With -fpic, the variable access will go directly and not via GOT.
>>>
>>> `clang/test/Driver/fdirect-access-external-data.c` is a driver test which 
>>> tests how the driver passes options to CC1.
>>
>>
>>
>>> `clang/lib/CodeGen/CodeGenModule.cpp` says how the CC1 option affects the 
>>> dso_local specifier in the LLVM IR output. Currently it is a no op for 
>>> -fpic/-fPIC.
>>
>> Great! Sorry I didnt realize this,  this is fine with me now!
>>
>>> (I have made some tests. It looks like implementing 
>>> -fno-direct-access-external-data for -fno-pic is not an insurmountable 
>>> work: ~50 tests)
>
> May I get a formal LGTM?  I am happy to give GCC devs a few more days... 
> https://gcc.gnu.org/pipermail/gcc/2021-January/234639.html (the previous 
> feature request and gcc/ mailing list message has given them one month and 
> -f[no-]direct-access-external-data is still the best name so far)

Ok, I can approve this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2482726 , @hoy wrote:

> In D93747#2482519 , @tmsriram wrote:
>
>> In D93747#2481494 , @dblaikie wrote:
>>
>>> In D93747#2481383 , @hoy wrote:
>>>
 In D93747#2481304 , @tmsriram 
 wrote:

> In D93747#2481223 , @tmsriram 
> wrote:
>
>> In D93747#2481163 , @dblaikie 
>> wrote:
>>
>>> In D93747#2481095 , @hoy wrote:
>>>
 In D93747#2481073 , @dblaikie 
 wrote:

> Suggesting that gdb isn't using the DW_AT_name at all for "break 
> " but instead demangling and stripping off the extras 
> from the linkage name, and since it can't demangle this uniquified 
> name (unlike the mangled name used when using the overloadable 
> attribute) that degrades the debugger user experience? I'd have to 
> test that in more detail/with some hand-hacked DWARF.

 Yes, I think in your case the linage name can be demangled by the 
 debugger. In my previous experiment, the uniquefied names could not be 
 demangled therefore I was not able to breakpoint.
>>>
>>> Ah, did some more testing. Seems it's because it isn't a classically 
>>> mangled name.
>
> The simplest fix I can think of is to emit the base 10 number of the 
> md5hash.  That would preserve all the existing uniqueness and be 
> demangler friendly.  @hoy @dblaikie  WDYT?

 I think using the base 10 form of md5hash is a smart workaround. Thanks 
 for the suggestion.
>>>
>>> Sure - wouldn't mind having some wording from the Itanium ABI/mangling 
>>> rules that explain/corroborate what we've seen from testing to ensure we 
>>> are using it correctly and there aren't any other ways that might be more 
>>> suitable, or lurking gotchas we haven't tested.
>>
>> Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
>>  it is now important that we don't generate 
>> the DW_AT_linkage_name for C style names using this suffix as it will not 
>> demangle.  I think this makes it important that we only update this field if 
>> it already exists.
>
> Yes, it makes sense to do the renaming only if the linkage name preexists to 
> not break the debuggability. Unfortunately we won't be able to support C with 
> this patch.

There is nothing needed to support C right?  overloadable attribute will mangle 
with _Z so it will work as expected?  What did I miss?

>>> It might be possible for this uniquifying step to check if the name is 
>>> mangled (does it start with "_Z") and if it isn't mangled, it could 
>>> mangle it (check the length of the name, then "_Zv") and 
>>> add the unique suffix. That looks to me like it preserves the original 
>>> debugging behavior?
>>>
>>> (has the problem that we don't actually know the mangling scheme at 
>>> this point - we do know it up in clang (where it might be Itanium 
>>> mangling or Microsoft mangling), might point again to the possibility 
>>> this feature is more suitable to implement in the frontend rather than 
>>> a middle end pass. And also the 'v' in the mangling is 'void return 
>>> type', which is a lie - not sure if we could do something better there)

 Doing name unification in the frontend sounds like the ultimate solution 
 and since the frontend has all the knowledge about the name mangler. I 
 think for now we can go with the solution @tmsriram suggested, i.e., using 
 the base 10 form of md5 hash.
>>>
>>> Any general idea of how long "for now" would be? It doesn't seem like 
>>> putting this off is going to make things especially better & seems like 
>>> more bug fixes/changes/etc are being built around the solution as it is at 
>>> the moment. So I'd be hesitant to put off this kind of restructuring too 
>>> long.
>
> I'm not sure. It looks like implementation in the front end has more 
> complexity as discussed in the other thread D94154 
> .
>
>>> I think it's pretty important that we don't break tradeoff 
>>> debuggability for profile accuracy. It'll make for a difficult tradeoff 
>>> for our/any users.
>>
>> Agreed, I didn't expect this.
>>
>>> (side note: using the original source file name seems problematic - I 
>>> know in google at least, the same source file is sometimes built into 
>>> more than one library/form with different preprocessor defines, so this 
>>> may not produce as unique a name as you are expecting?)
>>
>> It was a best effort and I 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481494 , @dblaikie wrote:

> In D93747#2481383 , @hoy wrote:
>
>> In D93747#2481304 , @tmsriram wrote:
>>
>>> In D93747#2481223 , @tmsriram 
>>> wrote:
>>>
 In D93747#2481163 , @dblaikie 
 wrote:

> In D93747#2481095 , @hoy wrote:
>
>> In D93747#2481073 , @dblaikie 
>> wrote:
>>
>>> Suggesting that gdb isn't using the DW_AT_name at all for "break 
>>> " but instead demangling and stripping off the extras 
>>> from the linkage name, and since it can't demangle this uniquified name 
>>> (unlike the mangled name used when using the overloadable attribute) 
>>> that degrades the debugger user experience? I'd have to test that in 
>>> more detail/with some hand-hacked DWARF.
>>
>> Yes, I think in your case the linage name can be demangled by the 
>> debugger. In my previous experiment, the uniquefied names could not be 
>> demangled therefore I was not able to breakpoint.
>
> Ah, did some more testing. Seems it's because it isn't a classically 
> mangled name.
>>>
>>> The simplest fix I can think of is to emit the base 10 number of the 
>>> md5hash.  That would preserve all the existing uniqueness and be demangler 
>>> friendly.  @hoy @dblaikie  WDYT?
>>
>> I think using the base 10 form of md5hash is a smart workaround. Thanks for 
>> the suggestion.
>
> Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules 
> that explain/corroborate what we've seen from testing to ensure we are using 
> it correctly and there aren't any other ways that might be more suitable, or 
> lurking gotchas we haven't tested.

Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
 it is now important that we don't generate 
the DW_AT_linkage_name for C style names using this suffix as it will not 
demangle.  I think this makes it important that we only update this field if it 
already exists.

> It might be possible for this uniquifying step to check if the name is 
> mangled (does it start with "_Z") and if it isn't mangled, it could 
> mangle it (check the length of the name, then "_Zv") and 
> add the unique suffix. That looks to me like it preserves the original 
> debugging behavior?
>
> (has the problem that we don't actually know the mangling scheme at this 
> point - we do know it up in clang (where it might be Itanium mangling or 
> Microsoft mangling), might point again to the possibility this feature is 
> more suitable to implement in the frontend rather than a middle end pass. 
> And also the 'v' in the mangling is 'void return type', which is a lie - 
> not sure if we could do something better there)
>>
>> Doing name unification in the frontend sounds like the ultimate solution and 
>> since the frontend has all the knowledge about the name mangler. I think for 
>> now we can go with the solution @tmsriram suggested, i.e., using the base 10 
>> form of md5 hash.
>
> Any general idea of how long "for now" would be? It doesn't seem like putting 
> this off is going to make things especially better & seems like more bug 
> fixes/changes/etc are being built around the solution as it is at the moment. 
> So I'd be hesitant to put off this kind of restructuring too long.
>
> I think it's pretty important that we don't break tradeoff debuggability 
> for profile accuracy. It'll make for a difficult tradeoff for our/any 
> users.

 Agreed, I didn't expect this.

> (side note: using the original source file name seems problematic - I 
> know in google at least, the same source file is sometimes built into 
> more than one library/form with different preprocessor defines, so this 
> may not produce as unique a name as you are expecting?)

 It was a best effort and I think the hashing can be improved by using more 
 signals other than just the module name if needed.  For hard data though, 
 this does significantly improve performance which clearly comes from 
 better profile attribution so it does something.
>
> I'm OK with the idea that this helped the situation - but it doesn't seem 
> very principled/rigorous. Rather than a sliding scale of "better" I think 
> it'd be worth considering in more detail what would be required to make this 
> correct.
>
> Using the object file name may be more robust - also not perfect for all 
> build systems (one could imagine a distributed build system that /might/ be 
> able to get away with having the distributed builders always build a file 
> named x.o - only to have the distribution system rename the file to its 
> 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481223 , @tmsriram wrote:

> In D93747#2481163 , @dblaikie wrote:
>
>> In D93747#2481095 , @hoy wrote:
>>
>>> In D93747#2481073 , @dblaikie 
>>> wrote:
>>>
 In D93747#2481053 , @tmsriram 
 wrote:

> In D93747#2480442 , @dblaikie 
> wrote:
>
>> @tmsriram - any ideas what your case/example was like that might've 
>> caused degraded debugging experience? Would be good to understand if 
>> we're producing some bad DWARF with this patch/if there might be some 
>> way to avoid it (as it seems like gdb can handle mangled 
>> names/overloading in C in this example I've tried)
>
> I haven't seen anything that caused degraded debugging experience.  I am 
> interested in this as we do look at this field for the purposes of 
> profile attribtution for calls that are inlined.  My comment was that we 
> don't need to create this if it didn't already exist.  I am not fully 
> aware of what would happen if we did it all the time.

 Ah, sorry, I got confused as to who's comment I was reading. I see it was 
 @hoy who said:

> If set, the gdb debugger will use that field to match the user input and 
> set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
> prevents the debugger from setting a breakpoint based on source names 
> unless the user specifies a decorated name.
>
> Hence, this approach sounds like a workaround for us when the profile 
> quality matters more than debugging experience.

 So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
 with setting a linkage name preventing the debugger from setting a 
 breakpoint based on the source name?

 Suggesting that gdb isn't using the DW_AT_name at all for "break >>> name>" but instead demangling and stripping off the extras from the 
 linkage name, and since it can't demangle this uniquified name (unlike the 
 mangled name used when using the overloadable attribute) that degrades the 
 debugger user experience? I'd have to test that in more detail/with some 
 hand-hacked DWARF.
>>>
>>> Yes, I think in your case the linage name can be demangled by the debugger. 
>>> In my previous experiment, the uniquefied names could not be demangled 
>>> therefore I was not able to breakpoint.
>>
>> Ah, did some more testing. Seems it's because it isn't a classically mangled 
>> name.

The simplest fix I can think of is to emit the base 10 number of the md5hash.  
That would preserve all the existing uniqueness and be demangler friendly.  
@hoy @dblaikie  WDYT?

> Yep, that's the issue
>
>   $ c++filt _Z3foov.__uniq
>   foo() [clone .__uniq]
>   
>   $ c++filt _Z3foov.__uniq.123
>   foo() [clone .__uniq.123]
>   
>   $ c++filt._Z3foov.__uniq.123abc
>   _Z3foov.__uniq.123abc
>
> The demangler does not understand a mix of numeric and alpha-numeric. It can 
> be either but not both and md5hashes contain both.  So we might have to 
> process the hash string or do something different to keep it demangler 
> friendly.
>
>> It might be possible for this uniquifying step to check if the name is 
>> mangled (does it start with "_Z") and if it isn't mangled, it could mangle 
>> it (check the length of the name, then "_Zv") and add the 
>> unique suffix. That looks to me like it preserves the original debugging 
>> behavior?
>>
>> (has the problem that we don't actually know the mangling scheme at this 
>> point - we do know it up in clang (where it might be Itanium mangling or 
>> Microsoft mangling), might point again to the possibility this feature is 
>> more suitable to implement in the frontend rather than a middle end pass. 
>> And also the 'v' in the mangling is 'void return type', which is a lie - not 
>> sure if we could do something better there)
>>
>> I think it's pretty important that we don't break tradeoff debuggability for 
>> profile accuracy. It'll make for a difficult tradeoff for our/any users.
>
> Agreed, I didn't expect this.
>
>> (side note: using the original source file name seems problematic - I know 
>> in google at least, the same source file is sometimes built into more than 
>> one library/form with different preprocessor defines, so this may not 
>> produce as unique a name as you are expecting?)
>
> It was a best effort and I think the hashing can be improved by using more 
> signals other than just the module name if needed.  For hard data though, 
> this does significantly improve performance which clearly comes from better 
> profile attribution so it does something.




Repository:
  rG LLVM Github Monorepo

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481163 , @dblaikie wrote:

> In D93747#2481095 , @hoy wrote:
>
>> In D93747#2481073 , @dblaikie wrote:
>>
>>> In D93747#2481053 , @tmsriram 
>>> wrote:
>>>
 In D93747#2480442 , @dblaikie 
 wrote:

> @tmsriram - any ideas what your case/example was like that might've 
> caused degraded debugging experience? Would be good to understand if 
> we're producing some bad DWARF with this patch/if there might be some way 
> to avoid it (as it seems like gdb can handle mangled names/overloading in 
> C in this example I've tried)

 I haven't seen anything that caused degraded debugging experience.  I am 
 interested in this as we do look at this field for the purposes of profile 
 attribtution for calls that are inlined.  My comment was that we don't 
 need to create this if it didn't already exist.  I am not fully aware of 
 what would happen if we did it all the time.
>>>
>>> Ah, sorry, I got confused as to who's comment I was reading. I see it was 
>>> @hoy who said:
>>>
 If set, the gdb debugger will use that field to match the user input and 
 set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
 prevents the debugger from setting a breakpoint based on source names 
 unless the user specifies a decorated name.

 Hence, this approach sounds like a workaround for us when the profile 
 quality matters more than debugging experience.
>>>
>>> So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
>>> with setting a linkage name preventing the debugger from setting a 
>>> breakpoint based on the source name?
>>>
>>> Suggesting that gdb isn't using the DW_AT_name at all for "break >> name>" but instead demangling and stripping off the extras from the linkage 
>>> name, and since it can't demangle this uniquified name (unlike the mangled 
>>> name used when using the overloadable attribute) that degrades the debugger 
>>> user experience? I'd have to test that in more detail/with some hand-hacked 
>>> DWARF.
>>
>> Yes, I think in your case the linage name can be demangled by the debugger. 
>> In my previous experiment, the uniquefied names could not be demangled 
>> therefore I was not able to breakpoint.
>
> Ah, did some more testing. Seems it's because it isn't a classically mangled 
> name.

Yep, that's the issue

  $ c++filt _Z3foov.__uniq
  foo() [clone .__uniq]
  
  $ c++filt _Z3foov.__uniq.123
  foo() [clone .__uniq.123]
  
  $ c++filt._Z3foov.__uniq.123abc
  _Z3foov.__uniq.123abc

The demangler does not understand a mix of numeric and alpha-numeric. It can be 
either but not both and md5hashes contain both.  So we might have to process 
the hash string or do something different to keep it demangler friendly.

> It might be possible for this uniquifying step to check if the name is 
> mangled (does it start with "_Z") and if it isn't mangled, it could mangle it 
> (check the length of the name, then "_Zv") and add the unique 
> suffix. That looks to me like it preserves the original debugging behavior?
>
> (has the problem that we don't actually know the mangling scheme at this 
> point - we do know it up in clang (where it might be Itanium mangling or 
> Microsoft mangling), might point again to the possibility this feature is 
> more suitable to implement in the frontend rather than a middle end pass. And 
> also the 'v' in the mangling is 'void return type', which is a lie - not sure 
> if we could do something better there)
>
> I think it's pretty important that we don't break tradeoff debuggability for 
> profile accuracy. It'll make for a difficult tradeoff for our/any users.

Agreed, I didn't expect this.

> (side note: using the original source file name seems problematic - I know in 
> google at least, the same source file is sometimes built into more than one 
> library/form with different preprocessor defines, so this may not produce as 
> unique a name as you are expecting?)

It was a best effort and I think the hashing can be improved by using more 
signals other than just the module name if needed.  For hard data though, this 
does significantly improve performance which clearly comes from better profile 
attribution so it does something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2480442 , @dblaikie wrote:

>>> In D93747#2470178 , @tmsriram 
>>> wrote:
>>>
 In D93747#2469556 , @hoy wrote:

>> In D93656 , @dblaikie wrote:
>> Though the C case is interesting - it means you'll end up with C 
>> functions with the same DWARF 'name' but different linkage name. I don't 
>> know what that'll do to DWARF consumers - I guess they'll probably be 
>> OK-ish with it, as much as they are with C++ overloading. I think there 
>> are some cases of C name mangling so it's probably supported/OK-ish with 
>> DWARF Consumers. Wouldn't hurt for you to take a look/see what happens 
>> in that case with a debugger like gdb/check other cases of C name 
>> mangling to see what DWARF they end up creating (with both GCC and 
>> Clang).
>
> I did a quick experiment with C name managing with GCC and -flto. It 
> turns out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never 
> set for C programs. If set, the gdb debugger will use that field to match 
> the user input and set breakpoints. Therefore, giving 
> `DW_AT_linkage_name` a uniquefied name prevents the debugger from setting 
> a breakpoint based on source names unless the user specifies a decorated 
> name.
>
> Hence, this approach sounds like a workaround for us when the profile 
> quality matters more than debugging experience. I'm inclined to have it 
> under a switch. What do you think?

 Just a thought, we could always check if rawLinkageName is set and only 
 set it when it is not null.  That seems safe without needing the option. 
 Not a strong opinion.
>>>
>>> Was this thread concluded? Doesn't look like any check was added?
>>
>> Thanks for the detailed analysis! Moving forward I would like to see a more 
>> clear contract about debug linkage names between the compiler and debugger 
>> as a guideline to code cloning work. For this patch, I'm adding a switch for 
>> it with a default value "on" since AutoFDO and propeller are the primary 
>> consumers of the work here and they mostly care about profile quality more 
>> than debugging experience. But correct me if I'm wrong and I'll flip the 
>> default value.
>
> Presumably people are going to want to debug these binaries - I'm not sure 
> it's a "one or the other" situation as it sounds like @tmsriram was 
> suggesting by only modifying the linkage name when it's already set this 
> might produce a better debugging experience, if I'm following the discussion 
> correctly?
>
> But I'm pretty sure there are places where C supports name mangling, so I 
> wouldn't mind understanding the gdb behavior in more detail - perhaps there's 
> a way to make it work better.
>
> Using Clang's __attribute__((overloadable)) support, I was able to produce a 
> C program with mangled names, with DW_AT_linkage_names on those functions, 
> like this:
>
>   $ cat test.c
>   void __attribute__((overloadable)) f(int i) {
>   }
>   void __attribute__((overloadable)) f(long l) {
>   }
>   int main() {
> f(3);
> f(5l);
>   }
>   blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
>   blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out
>   a.out:  file format elf64-x86-64
>   
>   .debug_info contents:
>   0x: Compile Unit: length = 0x009e, format = DWARF32, version = 
> 0x0004, abbr_offset = 0x, addr_size = 0x08 (next unit at 0x00a2)
>   
>   0x000b: DW_TAG_compile_unit
> DW_AT_producer("clang version 12.0.0 
> (g...@github.com:llvm/llvm-project.git 
> 20013e0940dea465a173c3c7ce60f0e419242ef8)")
> DW_AT_language(DW_LANG_C99)
> DW_AT_name("test.c")
> DW_AT_stmt_list   (0x)
> DW_AT_comp_dir
> ("/usr/local/google/home/blaikie/dev/scratch")
> DW_AT_low_pc  (0x00401110)
> DW_AT_high_pc (0x0040114c)
>   
>   0x002a:   DW_TAG_subprogram
>   DW_AT_low_pc(0x00401110)
>   DW_AT_high_pc   (0x00401119)
>   DW_AT_frame_base(DW_OP_reg6 RBP)
>   DW_AT_linkage_name  ("_Z1fi")
>   DW_AT_name  ("f")
>   DW_AT_decl_file 
> ("/usr/local/google/home/blaikie/dev/scratch/test.c")
>   DW_AT_decl_line (1)
>   DW_AT_prototyped(true)
>   DW_AT_external  (true)
>   
>   0x0043: DW_TAG_formal_parameter
> DW_AT_location(DW_OP_fbreg -4)
> DW_AT_name("i")
> DW_AT_decl_file   
> ("/usr/local/google/home/blaikie/dev/scratch/test.c")
> 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

This change looks good to me but I would like @rnk to sign off.  My opinion is 
that the change is simple enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93082: Append ".__part." to all basic block section symbols.

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG34e70d722dfd: Append .__part. to every basic 
block section symbol. (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D93082?vs=313447=313587#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93082

Files:
  clang/test/CodeGen/basic-block-sections.c
  lld/test/ELF/lto/basic-block-sections.ll
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
  llvm/test/CodeGen/X86/basic-block-sections-clusters-branches.ll
  llvm/test/CodeGen/X86/basic-block-sections-clusters-eh.ll
  llvm/test/CodeGen/X86/basic-block-sections-clusters.ll
  llvm/test/CodeGen/X86/basic-block-sections-directjumps.ll
  llvm/test/CodeGen/X86/basic-block-sections-eh.ll
  llvm/test/CodeGen/X86/basic-block-sections-list.ll
  llvm/test/CodeGen/X86/basic-block-sections-listbb.ll
  llvm/test/CodeGen/X86/basic-block-sections-mir-parse.mir
  llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll
  llvm/test/CodeGen/X86/basic-block-sections.ll
  llvm/test/CodeGen/X86/basic-block-sections_2.ll
  llvm/test/CodeGen/X86/cfi-basic-block-sections-1.ll
  
llvm/test/CodeGen/X86/cfi-inserter-basic-block-sections-callee-save-registers.ll
  llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll
  llvm/test/DebugInfo/X86/basic-block-sections_1.ll

Index: llvm/test/DebugInfo/X86/basic-block-sections_1.ll
===
--- llvm/test/DebugInfo/X86/basic-block-sections_1.ll
+++ llvm/test/DebugInfo/X86/basic-block-sections_1.ll
@@ -16,33 +16,33 @@
 ; NO-SECTIONS: DW_AT_high_pc [DW_FORM_data4] ({{.*}})
 ; BB-SECTIONS: DW_AT_low_pc [DW_FORM_addr] (0x)
 ; BB-SECTIONS-NEXT: DW_AT_ranges [DW_FORM_sec_offset]
-; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.1"
-; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.2"
-; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.3"
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.__part.1"
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.__part.2"
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi.__part.3"
 ; BB-SECTIONS-NEXT: [{{.*}}) ".text"
 ; BB-SECTIONS-ASM: _Z3fooi:
 ; BB-SECTIONS-ASM: .Ltmp{{[0-9]+}}:
 ; BB-SECTIONS-ASM-NEXT: .loc 1 2 9 prologue_end
 ; BB-SECTIONS-ASM: .Ltmp{{[0-9]+}}:
 ; BB-SECTIONS-ASM-NEXT: .loc 1 2 7 is_stmt
-; BB-SECTIONS-ASM: _Z3fooi.1:
+; BB-SECTIONS-ASM: _Z3fooi.__part.1:
 ; BB-SECTIONS-ASM: .LBB_END0_{{[0-9]+}}:
-; BB-SECTIONS-ASM: .size	_Z3fooi.1, .LBB_END0_{{[0-9]+}}-_Z3fooi.1
-; BB-SECTIONS-ASM: _Z3fooi.2:
+; BB-SECTIONS-ASM: .size	_Z3fooi.__part.1, .LBB_END0_{{[0-9]+}}-_Z3fooi.__part.1
+; BB-SECTIONS-ASM: _Z3fooi.__part.2:
 ; BB-SECTIONS-ASM: .Ltmp{{[0-9]+}}:
 ; BB-SECTIONS-ASM-NEXT: .LBB_END0_{{[0-9]+}}:
-; BB-SECTIONS-ASM: .size	_Z3fooi.2, .LBB_END0_{{[0-9]+}}-_Z3fooi.2
-; BB-SECTIONS-ASM: _Z3fooi.3:
+; BB-SECTIONS-ASM: .size	_Z3fooi.__part.2, .LBB_END0_{{[0-9]+}}-_Z3fooi.__part.2
+; BB-SECTIONS-ASM: _Z3fooi.__part.3:
 ; BB-SECTIONS-ASM: .Ltmp{{[0-9]+}}:
 ; BB-SECTIONS-ASM-NEXT: .LBB_END0_{{[0-9]+}}:
-; BB-SECTIONS-ASM: .size	_Z3fooi.3, .LBB_END0_{{[0-9]+}}-_Z3fooi.3
+; BB-SECTIONS-ASM: .size	_Z3fooi.__part.3, .LBB_END0_{{[0-9]+}}-_Z3fooi.__part.3
 ; BB-SECTIONS-ASM: .Lfunc_end0:
 ; BB-SECTIONS-ASM: .Ldebug_ranges0:
-; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.1
+; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.__part.1
 ; BB-SECTIONS-ASM-NEXT:	.quad	.LBB_END0_{{[0-9]+}}
-; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.2
+; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.__part.2
 ; BB-SECTIONS-ASM-NEXT:	.quad	.LBB_END0_{{[0-9]+}}
-; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.3
+; BB-SECTIONS-ASM-NEXT:	.quad	_Z3fooi.__part.3
 ; BB-SECTIONS-ASM-NEXT:	.quad	.LBB_END0_{{[0-9]+}}
 ; BB-SECTIONS-ASM-NEXT:	.quad	.Lfunc_begin0
 ; BB-SECTIONS-ASM-NEXT:	.quad	.Lfunc_end0
Index: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll
===
--- llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll
+++ llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll
@@ -27,7 +27,7 @@
 
 ; CHECK-NOT:.cfi_lsda
 
-; CHECK-LABEL:main.1:
+; CHECK-LABEL:main.__part.1:
 ; CHECK-NEXT:   .cfi_startproc
 
 ; CHECK-NON-PIC-NEXT:   .cfi_personality 3, __gxx_personality_v0
@@ -38,7 +38,7 @@
 
 ; CHECK-NOT:.cfi_lsda
 
-; CHECK-LABEL:main.2:
+; CHECK-LABEL:main.__part.2:
 ; CHECK-NEXT:   .cfi_startproc
 
 ; CHECK-NON-PIC-NEXT:   .cfi_personality 3, __gxx_personality_v0
@@ -82,12 +82,12 @@
 
 ;; Verify @LPStart encoding for NON-PIC mode.
 ; CHECK-NON-PIC-NEXT:   .byte	0   # @LPStart Encoding = absptr
-; CHECK-NON-PIC-NEXT:   .quad	main.2
+; CHECK-NON-PIC-NEXT:   .quad	main.__part.2
 
 ;; Verify @LPStart encoding for PIC mode.
 ; CHECK-PIC-NEXT:   .byte	16 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2470189 , @hoy wrote:

> In D93747#2470178 , @tmsriram wrote:
>
>> In D93747#2469556 , @hoy wrote:
>>
 In D93656 , @dblaikie wrote:
 Though the C case is interesting - it means you'll end up with C functions 
 with the same DWARF 'name' but different linkage name. I don't know what 
 that'll do to DWARF consumers - I guess they'll probably be OK-ish with 
 it, as much as they are with C++ overloading. I think there are some cases 
 of C name mangling so it's probably supported/OK-ish with DWARF Consumers. 
 Wouldn't hurt for you to take a look/see what happens in that case with a 
 debugger like gdb/check other cases of C name mangling to see what DWARF 
 they end up creating (with both GCC and Clang).
>>>
>>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for 
>>> C programs. If set, the gdb debugger will use that field to match the user 
>>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>>> uniquefied name prevents the debugger from setting a breakpoint based on 
>>> source names unless the user specifies a decorated name.
>>>
>>> Hence, this approach sounds like a workaround for us when the profile 
>>> quality matters more than debugging experience. I'm inclined to have it 
>>> under a switch. What do you think?
>>
>> Just a thought, we could always check if rawLinkageName is set and only set 
>> it when it is not null.  That seems safe without needing the option. Not a 
>> strong opinion.
>
> It seems that the demangler of the debugger is not able to handle an 
> uniquefied name, even if the debug record originally comes with a linkage 
> name.

Yep, the demangler (which can be checked with c++filt) is very restricted on 
what comes after the "."  It can be either numbers or characters but not a mix 
of the two.  I guess we can enhance it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2469556 , @hoy wrote:

>> In D93656 , @dblaikie wrote:
>> Though the C case is interesting - it means you'll end up with C functions 
>> with the same DWARF 'name' but different linkage name. I don't know what 
>> that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, 
>> as much as they are with C++ overloading. I think there are some cases of C 
>> name mangling so it's probably supported/OK-ish with DWARF Consumers. 
>> Wouldn't hurt for you to take a look/see what happens in that case with a 
>> debugger like gdb/check other cases of C name mangling to see what DWARF 
>> they end up creating (with both GCC and Clang).
>
> I did a quick experiment with C name managing with GCC and -flto. It turns 
> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C 
> programs. If set, the gdb debugger will use that field to match the user 
> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
> uniquefied name prevents the debugger from setting a breakpoint based on 
> source names unless the user specifies a decorated name.
>
> Hence, this approach sounds like a workaround for us when the profile quality 
> matters more than debugging experience. I'm inclined to have it under a 
> switch. What do you think?

Just a thought, we could always check if rawLinkageName is set and only set it 
when it is not null.  That seems safe without needing the option. Not a strong 
opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:26-29
+static cl::opt UniqueDebugAndProfileNames(
+"unqiue-debug-profile-names", cl::init(true), cl::Hidden,
+cl::desc("Uniqueify debug and profile symbol Names"));
+

hoy wrote:
> dblaikie wrote:
> > Do you have plans to use the flag in testing, etc? If not, I guess you 
> > might as well bake in the behavior you want if no one else is using 
> > this/there's no current use case for the flexibility?
> I'm not quite sure if updating debug names are required in all use cases. 
> @tmsriram do you think we can just remove the flag?
I vote for removing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2056-2059
+  void replaceRawLinkageName(MDString *LinkageName) {
+replaceOperandWith(3, LinkageName);
+  }
+

dblaikie wrote:
> The need to add this API makes me a bit uncertain that it's the right 
> direction - any chance you could find other examples of Function duplication 
> in LLVM passes (maybe the partial inliner? Perhaps when it partially inlines 
> an external function it has to clone the function so the external version 
> remains identical?) and how they deal with debug info? Perhaps there's an 
> existing API/mechanism to update the DISubprogram as you want without adding 
> this.
This does not seem like a radical new API to me.  We could use existing 
setOperand or replaceOperandWith here but need a public wrapper to expose it, 
just like getRawLinkageName.  For example, DICompositeType is mutated like this 
with buildODRType.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:881
+void DISubprogram::replaceRawLinkageName(MDString *LinkageName) {
+  replaceOperandWith(3, LinkageName);
+}

Nit, Move the body to DebugInfoMetadata.h itself? 



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:51
+  auto Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+}

hoy wrote:
> tmsriram wrote:
> > Do we need to check if it had a rawLinkageName before replacing it?  
> Good point. `rawLinkageName` is missing for C programs. I think it might 
> still be appropriate to assign a new linkage name in the case so as to differ 
> from the original source function name. What do you think?
@dblaikie Adding the expert here.  As far as I understand, the linkage name is 
the mangled name and this would capture this too correctly. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

https://reviews.llvm.org/D73307#1932131  rnk@ mentioned this :  "At a higher 
level, should this just be an IR pass that clang adds into the pipeline when 
the flag is set? It should be safe to rename internal functions and give 
private functions internal linkage. It would be less invasive to clang and have 
better separation of concerns. As written, this is based on the source filename 
on the module, which is accessible from IR. The only reason I can think of 
against this is that the debug info might refer to the function linkage name, 
but maybe that is calculated later during codegen."

I just wanted to mention it  here that this was anticipated and was missed in 
the original patch, my bad as I didnt think about DebugInfo change.  However, I 
think it is pretty straightforward to change the linkage name so I would still 
keep the pass approach.




Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:27
+static cl::opt UniqueDebugAndProfileNames(
+"unqiue-debug-profile-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify debug and profile symbol Names"));

Can we make it true by default?  Atleast the DW_AT_linkage_name must reflect 
the new name by default IMO.



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:51
+  auto Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+}

Do we need to check if it had a rawLinkageName before replacing it?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

I too was currently working on changing the raw linkage names, 
DW_AT_linkage_name, and was about to send out a patch along these lines :).  
Thanks for doing this!  I will take a look at it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D92633#2434766 , @MaskRay wrote:

> In D92633#2434714 , @tmsriram wrote:
>
>> Correct me if I am wrong, but I do see that this behavior is touched.  Line 
>> 10 in -fdirect-access-external-data.c :
>>
>> // RUN: %clang -### -c -target aarch64 %s -fpic 
>> -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT
>>
>> With -fpic, the variable access will go directly and not via GOT.
>
> `clang/test/Driver/fdirect-access-external-data.c` is a driver test which 
> tests how the driver passes options to CC1.



> `clang/lib/CodeGen/CodeGenModule.cpp` says how the CC1 option affects the 
> dso_local specifier in the LLVM IR output. Currently it is a no op for 
> -fpic/-fPIC.

Great! Sorry I didnt realize this,  this is fine with me now!

> (I have made some tests. It looks like implementing 
> -fno-direct-access-external-data for -fno-pic is not an insurmountable work: 
> ~50 tests)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D92633#2434511 , @MaskRay wrote:

> In D92633#2434337 , @tmsriram wrote:
>
>> In D92633#2434267 , @MaskRay wrote:
>>
>>> In D92633#2434231 , @tmsriram 
>>> wrote:
>>>
 Sorry just one more thing which is a bit concerning:

 When I do  :

 $ clang -fPIC -frxternal-data-access foo.c

 You will omit the GOT but this object can go into a shared object and 
 break the build as this does not apply to shared objects?  Should we allow 
 this at all for -fPIC?  I dont see a need for this but if you want to,  
 maybe warn?  The user should know that this cannot go into a shared object 
 right?
 I am also ok with commenting that this option can do bad things for -fPIC 
 and the user should know what they are doing.
>>>
>>> `clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie 
>>> a.o` (producing an executable with either `-no-pie` or `-pie`) is fine.
>>
>> Yes, agreed. Putting this object into an executable (pie/no-pie) no matter 
>> how you compile it  is always fine as long as the backend supports copy 
>> relocations. No issues there.
>>
>>> For `-shared`, because an ELF linker assumes interposition for non-local 
>>> STV_DEFAULT symbols by default, linking such an object files requires some 
>>> mechanism to make it non-preemptible.
>>
>> Right, that was my point.  Without -fPIC, we can be guaranteed that it won't 
>> go into a shared object and this case wouldn't arise.
>>
>>> The simplest is `clang -fPIC -fdirect-access-external-data -c a.c; ld 
>>> -shared -Bsymbolic a.o`
>>> Other mechanisms include: `--dynamic-list` (don't list the symbol) and 
>>> `--version-script` (localize the symbol).
>>> This is going to be tricky and I don't know how to fit the information into 
>>> the few-line description of the option.
>>
>> How about making this option applicable for -fpie/fPIE and the default case 
>> and *not allowing* it for -fPIC/-fpic?   That seems the safest approach.
>>
>>> I just checked how to make -fdirect-access-eternal-data work for -fno-pic 
>>> (both TargetMachine::shouldAssumeDSOLocal and 
>>> CodeGenModule.cpp:shouldAssumeDSOLocal should not assume false for 
>>> Reloc::Static), unfortunately there are 200 tests needing updates. (This 
>>> reminds me that LLVM doesn't get the default for dso_local right, so many 
>>> tests have `dso_local` in ELF/COFF but no `dso_local` in Mach-O. 
>>> Unfortunately it is extremely difficult to fix it today (D76561 
>>> ))
>>
>> Ok, I lost you here a bit.  This should be fine for -fno-pic was my 
>> understanding.
>>
>> Let me try to summarize the motivations of this patch:
>>
>> 1. Originally, the default build (fno-pie/fno-pic), always used direct 
>> access for external globals resulting in copy relocations if it is truly 
>> external at link time.  You want to change that to be able to not have copy 
>> relocations with -fno-direct-access-external-data, and you claim this would 
>> be useful for POWER, great!
>> 2. Originally, with -fPIE and -fpie, -mpie-copy-relocations allowed direct 
>> access to externals.  This was always safe because the object was guaranteed 
>> to go into an executable.  The new option preserves this behavior so there 
>> is **no concern**.
>
> Yes for 1 and 2. This patch only makes the options work for -fpie (as the 
> original -mpie-copy-relocations does).
>
> 1 will be a nice cleanup (to drop 2 lines from 
> TargetMachine::shouldAssumeDSOLocal) but it may require some test updates and 
> it looks like `CodeGen/MachineCSE.cpp` exposes a crashing bug that it cannot 
> handle non-dso_local `external constant` in -relocation-model=static mode 
> correctly...
>
>> 3. Originally, there was no way to generate direct accesses to external 
>> global variables with -fPIC/-fpic. That behavior will change if you support 
>> -fdirect-access-external-data with -fPIC. **That is my concern that this 
>> adds to the complexity and the user should know what they are doing.**
>>
>> Given 3), I am wondering if you really need this patch.  I will leave it to 
>> you but please do consider the fact that opening up this option to -fPIC 
>> might be a problem.
>
> I agree. The behavior is not touched in this patch.

Correct me if I am wrong, but I do see that this behavior is touched.  Line 10 
in -fdirect-access-external-data.c :

// RUN: %clang -### -c -target aarch64 %s -fpic -fdirect-access-external-data 
2>&1 | FileCheck %s --check-prefix=DIRECT

With -fpic, the variable access will go directly and not via GOT.

> I think the existing -mpie-copy-relocations users should be aware that the 
> option (-fdirect-access-external-data) should generally only be used with 
> -fno-pic and -fpie.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D92633#2434267 , @MaskRay wrote:

> In D92633#2434231 , @tmsriram wrote:
>
>> Sorry just one more thing which is a bit concerning:
>>
>> When I do  :
>>
>> $ clang -fPIC -frxternal-data-access foo.c
>>
>> You will omit the GOT but this object can go into a shared object and break 
>> the build as this does not apply to shared objects?  Should we allow this at 
>> all for -fPIC?  I dont see a need for this but if you want to,  maybe warn?  
>> The user should know that this cannot go into a shared object right?
>> I am also ok with commenting that this option can do bad things for -fPIC 
>> and the user should know what they are doing.
>
> `clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie 
> a.o` (producing an executable with either `-no-pie` or `-pie`) is fine.

Yes, agreed. Putting this object into an executable (pie/no-pie) no matter how 
you compile it  is always fine as long as the backend supports copy 
relocations. No issues there.

> For `-shared`, because an ELF linker assumes interposition for non-local 
> STV_DEFAULT symbols by default, linking such an object files requires some 
> mechanism to make it non-preemptible.

Right, that was my point.  Without -fPIC, we can be guaranteed that it won't go 
into a shared object and this case wouldn't arise.

> The simplest is `clang -fPIC -fdirect-access-external-data -c a.c; ld -shared 
> -Bsymbolic a.o`
> Other mechanisms include: `--dynamic-list` (don't list the symbol) and 
> `--version-script` (localize the symbol).
> This is going to be tricky and I don't know how to fit the information into 
> the few-line description of the option.

How about making this option applicable for -fpie/fPIE and the default case and 
*not allowing* it for -fPIC/-fpic?   That seems the safest approach.

> I just checked how to make -fdirect-access-eternal-data work for -fno-pic 
> (both TargetMachine::shouldAssumeDSOLocal and 
> CodeGenModule.cpp:shouldAssumeDSOLocal should not assume false for 
> Reloc::Static), unfortunately there are 200 tests needing updates. (This 
> reminds me that LLVM doesn't get the default for dso_local right, so many 
> tests have `dso_local` in ELF/COFF but no `dso_local` in Mach-O. 
> Unfortunately it is extremely difficult to fix it today (D76561 
> ))

Ok, I lost you here a bit.  This should be fine for -fno-pic was my 
understanding.

Let me try to summarize the motivations of this patch:

1. Originally, the default build (fno-pie/fno-pic), always used direct access 
for external globals resulting in copy relocations if it is truly external at 
link time.  You want to change that to be able to not have copy relocations 
with -fno-direct-access-external-data, and you claim this would be useful for 
POWER, great!
2. Originally, with -fPIE and -fpie, -mpie-copy-relocations allowed direct 
access to externals.  This was always safe because the object was guaranteed to 
go into an executable.  The new option preserves this behavior so there is **no 
concern**.
3. Originally, there was no way to generate direct accesses to external global 
variables with -fPIC/-fpic. That behavior will change if you support 
-fdirect-access-external-data with -fPIC. **That is my concern that this adds 
to the complexity and the user should know what they are doing.**

Given 3), I am wondering if you really need this patch.  I will leave it to you 
but please do consider the fact that opening up this option to -fPIC might be a 
problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D92633#2434212 , @MaskRay wrote:

> In D92633#2434166 , @tmsriram wrote:
>
>> LGTM.  I checked it completely and the patch makes total sense to me.
>
> Thanks! The name bothers me a bit. There are two parts "direct-access" 
> (direct access relocations like absolute relocations and PC-relative 
> relocations) "external-data" (this is for an external global data symbol with 
> default visibility) (-fno-plt is a similar option for function symbols)
>
> "external global data symbol with default visibility" is apparently too long 
> and not suitable for an option name.
> I do not know how to fit `default` into the name, so I just omit it.
> I think `external` implies `global` (undefined local symbols are not allowed, 
> either by the assembler or by the linker in various binary formats), so 
> `external-global` probably does not convey more information than 
> `external-data`. In the end I still think `-fdirect-access-external-data` is 
> the best name I can think of...

Acknowledged.  To me, global is more specific but I do not want to push this 
any further. Feel free to keep the current choice :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Sorry just one more thing which is a bit concerning:

When I do  :

$ clang -fPIC -frxternal-data-access foo.c

You will omit the GOT but this object can go into a shared object and break the 
build as this does not apply to shared objects?  Should we allow this at all 
for -fPIC?  I dont see a need for this but if you want to,  maybe warn?  The 
user should know that this cannot go into a shared object right?
I am also ok with commenting that this option can do bad things for -fPIC and 
the user should know what they are doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

LGTM.  I checked it completely and the patch makes total sense to me.




Comment at: clang/include/clang/Driver/Options.td:1866
 def fno_pie : Flag<["-"], "fno-pie">, Group;
+def fdirect_access_external_data : Flag<["-"], 
"fdirect-access-external-data">, Group, Flags<[CC1Option]>,
+  HelpText<"Don't use GOT indirection to reference external data symbols">;

Would -fdirect-access-external-global be a more appropriate name?, data sounds 
generic to me but I don't have a strong opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D92633#2433979 , @MaskRay wrote:

> In D92633#2433108 , @tmsriram wrote:
>
>> You said : "The name -mpie-copy-relocations is misleading [1] and does not 
>> capture the idea that this option can actually apply to all of 
>> -fno-pic,-fpie, ..."
>>
>> Could you please clarify why this option needs to apply to -fno-pic?  Here 
>> is what I tried with trunk clang:
>
> If the user wants to guarantee no copy relocations in -fno-pic code, they can 
> theoretically apply -fno-direct-access-external-data to use a GOT indirection.
> This is not implemented, though.
>
>> extern int var;
>> int get() { return var; }
>>
>> $ clang -S foo.c -o -
>> 
>>  movlvar, %eax
>>  popq%rbp
>> ...
>>
>> With -fno-pic,  this will never need to use -mpie-copy-relocations at all, 
>> so this case is not relevant right?  Did I miss anything?
>
> -fno-pic code can only be used with -no-pie links (position-dependent 
> executables) If var is not defined in the linked executable, it will have a 
> copy relocation.

Thanks for explaining.  I know that by default (i.e. no-pic and no-pie), copy 
relocations will be used for external data accesses.  So, you are saying that 
you are adding a mechanism to disable copy relocations for the no-pic/no-pie 
case too. Is there a need for this, purely a question.  I know that copy 
relocations are frowned upon so maybe there was a feature request. If so, 
citing that would make it more clear. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

You said : "The name -mpie-copy-relocations is misleading [1] and does not 
capture the idea that this option can actually apply to all of -fno-pic,-fpie, 
..."

Could you please clarify why this option needs to apply to -fno-pic?  Here is 
what I tried with trunk clang:

extern int var;
int get() { return var; }

$ clang -S foo.c -o -

movlvar, %eax
popq%rbp
...

With -fno-pic,  this will never need to use -mpie-copy-relocations at all, so 
this case is not relevant right?  Did I miss anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92633

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

[PATCH] D90815: -fbasic-block-sections=list=: Suppress output if failed to open the file

[Sriraman Tallam via Phabricator via cfe-commits]

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

Thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90815

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

[PATCH] D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad1b9daa4bf4: Prepend __uniq to symbol names 
hash with -funique-internal-linkage-names. (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89617

Files:
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll


Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
===
--- llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
@@ -9,5 +9,5 @@
   ret i32 0
 }
 
-; CHECK: @glob.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
-; CHECK: define internal i32 @foo.6ae72bb15a7d1834b42ae042a58f7a4d()
+; CHECK: @glob.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
+; CHECK: define internal i32 @foo.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d()
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -27,7 +27,9 @@
   Md5.final(R);
   SmallString<32> Str;
   llvm::MD5::stringifyResult(R, Str);
-  std::string ModuleNameHash = (Twine(".") + Twine(Str)).str();
+  // Prepend "__uniq" before the hash for tools like profilers to understand 
that
+  // this symbol is of internal linkage type.
+  std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
 
   // Append the module hash to all internal linkage functions.
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
@@ -53,15 +53,15 @@
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
-// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
-// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: @_ZL4glob.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9a-f]+}}
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
-// UNIQUEO1: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
 // UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUEO1: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
-// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
+// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}


Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
===
--- llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
@@ -9,5 +9,5 @@
   ret i32 0
 }
 
-; CHECK: @glob.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
-; CHECK: define internal i32 @foo.6ae72bb15a7d1834b42ae042a58f7a4d()
+; CHECK: @glob.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
+; CHECK: define internal i32 @foo.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d()
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -27,7 +27,9 @@
   Md5.final(R);
   SmallString<32> Str;
   llvm::MD5::stringifyResult(R, Str);
-  std::string ModuleNameHash = (Twine(".") + Twine(Str)).str();
+  // Prepend "__uniq" before the hash for tools like profilers to understand that
+  // this symbol is of internal linkage type.
+  std::string 

[PATCH] D89500: Fix the error message with -fbasic-block-sections=list=

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf88785460ecf: Improve file doesnt exist error with 
-fbasic-block-sections= (authored by tmsriram).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89500

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -4,9 +4,14 @@
 // RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LABELS %s
 // RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 // RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=alll %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list= %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-NULL-LIST %s
 //
 // CHECK-OPT-NONE:   "-fbasic-block-sections=none"
 // CHECK-OPT-ALL:"-fbasic-block-sections=all"
 // CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
 // CHECK-TRIPLE: error: unsupported option '-fbasic-block-sections=all' 
for target
+// CHECK-INVALID-VALUE: error: invalid value {{[^ ]*}} in 
'-fbasic-block-sections={{.*}}'
+// CHECK-OPT-NULL-LIST: "-fbasic-block-sections=list="
Index: clang/test/CodeGen/basic-block-sections.c
===
--- clang/test/CodeGen/basic-block-sections.c
+++ clang/test/CodeGen/basic-block-sections.c
@@ -6,6 +6,7 @@
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=all -o - < %s | 
FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
 // RUN: %clang_cc1 -triple x86_64 -S 
-fbasic-block-sections=list=%S/Inputs/basic-block-sections.funcnames -o - < %s 
| FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=all 
-funique-basic-block-section-names -o - < %s | FileCheck %s 
--check-prefix=UNIQUE
+// RUN: not %clang_cc1 -fbasic-block-sections=list= -emit-obj %s 2>&1 | 
FileCheck %s --check-prefix=ERROR
 
 int world(int a) {
   if (a > 10)
@@ -38,3 +39,4 @@
 //
 // UNIQUE: .section .text.world.world.1,
 // UNIQUE: .section .text.another.another.1,
+// ERROR: error:  unable to load basic block sections function list: 'No such 
file or directory'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4924,7 +4924,7 @@
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
   StringRef Val = A->getValue();
   if (Val != "all" && Val != "labels" && Val != "none" &&
-  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+  !Val.startswith("list="))
 D.Diag(diag::err_drv_invalid_value)
 << A->getAsString(Args) << A->getValue();
   else


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -4,9 +4,14 @@
 // RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
 // RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 // RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=alll %s -S 2>&1 | FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list %s -S 2>&1 | FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list= %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NULL-LIST %s
 //
 // CHECK-OPT-NONE:   "-fbasic-block-sections=none"
 // CHECK-OPT-ALL:"-fbasic-block-sections=all"
 // CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
 // CHECK-TRIPLE: error: unsupported option '-fbasic-block-sections=all' for target
+// CHECK-INVALID-VALUE: error: invalid value {{[^ ]*}} 

[PATCH] D87921: Fix -funique-internal-linkage-names to work with -O2 and new pass manager

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6950db36d33d: The wrong placement of add pass with 
optimizations led to -funique-internal… (authored by tmsriram).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D87921?vs=292852=293208#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87921

Files:
  clang/lib/CodeGen/BackendUtil.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
@@ -1,8 +1,10 @@
 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck 
%s --check-prefix=PLAIN
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm 
-funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | 
FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm 
-funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm 
-funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | 
FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | 
FileCheck %s --check-prefix=UNIQUEO1
 
 static int glob;
 static int foo() {
@@ -59,3 +61,7 @@
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
 // UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUEO1: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1262,12 +1262,6 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
-  // Add UniqueInternalLinkageNames Pass which renames internal linkage
-  // symbols with unique names.
-  if (CodeGenOpts.UniqueInternalLinkageNames) {
-MPM.addPass(UniqueInternalLinkageNamesPass());
-  }
-
   // Lastly, add semantically necessary passes for LTO.
   if (IsLTO || IsThinLTO) {
 MPM.addPass(CanonicalizeAliasesPass());
@@ -1363,12 +1357,6 @@
   MPM.addPass(InstrProfiling(*Options, false));
 });
 
-  // Add UniqueInternalLinkageNames Pass which renames internal linkage
-  // symbols with unique names.
-  if (CodeGenOpts.UniqueInternalLinkageNames) {
-MPM.addPass(UniqueInternalLinkageNamesPass());
-  }
-
   if (IsThinLTO) {
 MPM = PB.buildThinLTOPreLinkDefaultPipeline(
 Level, CodeGenOpts.DebugPassManager);
@@ -1385,6 +1373,11 @@
   }
 }
 
+// Add UniqueInternalLinkageNames Pass which renames internal linkage
+// symbols with unique names.
+if (CodeGenOpts.UniqueInternalLinkageNames)
+  MPM.addPass(UniqueInternalLinkageNamesPass());
+
 if (CodeGenOpts.MemProf) {
   MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
   MPM.addPass(ModuleMemProfilerPass());


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
@@ -1,8 +1,10 @@
 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Sriraman Tallam via Phabricator via cfe-commits]

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

This LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D85408#2253055 , @MaskRay wrote:

> I am still reading the patch, but I have noticed one thing worth discussing. 
> `.bb_addr_map` is a non-SHF_ALLOC section (meaning that it is not part of the 
> memory image). An absolute relocation type (`.quad .Lfunc_begin0`) works but 
> the value is a link-time address, not taking account of the load base 
> (PIE/shared object)). (If .bb_addr_map has the SHF_ALLOC flag, linkers will 
> report a text relocation) How do you intend to use `.bb_addr_map` at runtime?

bb_addr_map is intentionally not loaded as it is never meant to be used at 
run-time. bb_addr_map will be later be used in conjunction with a hardware 
profile of the binary to associate executed basic block addresses to machine 
basic blocks.  The hardware profile of the binary has enough info to determine 
the load base address with PIE, this is pretty straight forward and multiple 
strategies exist.  What exactly is the concern here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408

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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
tmsriram marked 2 inline comments as done.
Closed by commit rGe0bca46b0854: Options for Basic Block Sections, enabled in 
D68063 and D73674. (authored by tmsriram).

Changed prior to commit:
  https://reviews.llvm.org/D68049?vs=264733=267811#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -879,7 +879,7 @@
 Name += MBB.getParent()->getName();
   } else {
 Name += MBB.getParent()->getSection()->getName();
-if (TM.getUniqueBBSectionNames()) {
+if (TM.getUniqueBasicBlockSectionNames()) {
   Name += ".";
   Name += MBB.getSymbol()->getName();
 } else {
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 void MachineFunction::createBBLabels() {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -78,7 +78,7 @@
 CGOPT(unsigned, TLSSize)
 CGOPT(bool, EmulatedTLS)
 CGOPT(bool, UniqueSectionNames)
-CGOPT(bool, UniqueBBSectionNames)
+CGOPT(bool, UniqueBasicBlockSectionNames)
 CGOPT(EABI, EABIVersion)
 CGOPT(DebuggerKind, DebuggerTuningOpt)
 CGOPT(bool, EnableStackSizeSection)
@@ -350,11 +350,11 @@
   cl::init(true));
   CGBINDOPT(UniqueSectionNames);
 
-  static cl::opt UniqueBBSectionNames(
+  static cl::opt UniqueBasicBlockSectionNames(
   "unique-bb-section-names",
   cl::desc("Give unique names to every basic block section"),
   cl::init(false));
-  CGBINDOPT(UniqueBBSectionNames);
+  CGBINDOPT(UniqueBasicBlockSectionNames);
 
   static cl::opt EABIVersion(
   "meabi", cl::desc("Set EABI type (default depends on triple):"),
@@ -460,7 +460,7 @@
   Options.FunctionSections = getFunctionSections();
   Options.BBSections = getBBSectionsMode(Options);
   Options.UniqueSectionNames = getUniqueSectionNames();
-  Options.UniqueBBSectionNames = getUniqueBBSectionNames();
+  Options.UniqueBasicBlockSectionNames = getUniqueBasicBlockSectionNames();
   Options.TLSSize = getTLSSize();
   Options.EmulatedTLS = getEmulatedTLS();
   Options.ExplicitEmulatedTLS = EmulatedTLSView->getNumOccurrences() > 0;
Index: llvm/lib/CodeGen/BBSectionsPrepare.cpp
===
--- llvm/lib/CodeGen/BBSectionsPrepare.cpp
+++ llvm/lib/CodeGen/BBSectionsPrepare.cpp
@@ -9,15 +9,15 @@
 // BBSectionsPrepare implementation.
 //
 // The purpose of this pass is to assign sections to basic blocks when
-// -fbasicblock-sections= option is used. Further, with profile information only
-// the subset of basic blocks with profiles are placed in separate sections and
-// the rest are grouped in a cold section. The exception handling blocks are
+// -fbasic-block-sections= option is used. Further, with profile information
+// only the subset of basic blocks with profiles are placed in separate sections
+// and the rest are grouped in a cold section. The exception handling blocks are
 // treated specially to ensure they are all in one seciton.
 //
 // Basic Block Sections
 // 
 //
-// With option, -fbasicblock-sections=list, every function may be split into
+// With option, -fbasic-block-sections=list, every 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

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

In D68049#2066937 , @MaskRay wrote:

> LLD side changes look good. Are you waiting on an explicit approval from 
> @rmisth ?


Yes.




Comment at: lld/ELF/LTO.cpp:79
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

MaskRay wrote:
> `--lto-basic-block-sections`
> 
> I think it should be fine updating it here, and probably preferable, since 
> you already touched some related LLD code. It does not affect any tests. (You 
> did not add any tests for `--lto-basicblock-sections` in the original LLD 
> patch.)
Yes, I intend to clean this up and the llc option in a separate cleanup patch 
so that this doesn't get too complicated.  Is that alright?


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:1336
+
+Generate labels for each basic block or place each basic block or a subset of 
basic blocks in its own section
+

rsmith wrote:
> This file is automatically generated from the .td file; this text will be 
> lost when the file is auto-generated. To include a custom description here, 
> it should be specified as a `DocBrief` annotation on the option. (Otherwise 
> we default to using the `HelpText`.)
Thanks, I included it in both places like other options seem to be doing.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 264733.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Address reviewer comments:

- New diagnostic kind
- Document list=
- Add docbrief


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

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,44 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  With the ``list=`` option, a file containing the subset of basic blocks
+  that need to placed in unique sections can be specified.  The format of the
+  file is as follows.  For example, ``list=spec.txt`` where ``spec.txt`` is the
+  following:
+
+  ::
+
+!foo
+!!2
+!_Z3barv
+
+  will place the machine basic block with ``id 2`` in function ``foo`` in a
+  unique section.  It will also place all basic blocks of functions ``bar``
+  in unique sections.
+
+  Further, section clusters can also be specified using the ``list=``
+  option.  For example, ``list=spec.txt`` where ``spec.txt`` contains:
+
+  ::
+
+!foo
+!!1 !!3 !!5
+!!2 !!4 !!6
+
+  will create two unique sections for function ``foo`` with the first
+  containing the odd numbered basic blocks and the second containing the
+  even numbered basic blocks.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section.
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -929,7 +929,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -78,7 +78,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
   

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.




Comment at: clang/include/clang/Basic/CodeGenOptions.h:114-127
+  // -fbasic-block-sections=.  The allowed values with this option are:
+  // {"labels", "all", "", "none"}.
+  //
+  // "labels": Only generate basic block symbols (labels) for all basic
+  //   blocks, do not generate unique sections for basic blocks.
+  //   Use the machine basic block id in the symbol name to
+  //   associate profile info from virtual address to machine

rsmith wrote:
> This comment appears to be out of date with respect to the `list=` change.
Fixed, sorry.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 263371.
tmsriram marked 5 inline comments as done and an inline comment as not done.
tmsriram added a comment.

Address reviewer comments:

1. Use Diagnostic instead of errs
2. Move test input to Inputs/
3. Fix option description.


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

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,16 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: lld/ELF/LTO.cpp:80
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

shenhan wrote:
> Maybe "--lto-basicblock-sections" should be changed to 
> "--lto-basic-block-sections" too?
Ok if I do this as a separate patch?  llc options need to renamed too so I 
clean up independently after this.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 263322.
tmsriram added a comment.

Rebase.


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

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basic-block-sections.c
  clang/test/CodeGen/basic-block-sections.funcnames
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,16 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 void MachineFunction::createBBLabels() {
Index: llvm/lib/CodeGen/BBSectionsPrepare.cpp
===
--- 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe8147ad82226: Uniuqe Names for Internal Linkage Symbols. 
(authored by tmsriram).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,61 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// 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 i32 @_ZL4mverv()
+// PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -957,6 +957,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4253,6 +4253,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_linkage_names,
+options::OPT_fno_unique_internal_linkage_names,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4872,6 +4874,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_linkage_names,
+   options::OPT_fno_unique_internal_linkage_names, false))
+CmdArgs.push_back("-funique-internal-linkage-names");
+
   

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 262797.
tmsriram added a comment.

Update patch with changes to BackendUtil.cpp (forgot this file).


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,61 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// 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 i32 @_ZL4mverv()
+// PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -77,6 +77,7 @@
 #include "llvm/Transforms/Utils/EntryExitInstrumenter.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
+#include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include 
 using namespace clang;
 using namespace llvm;
@@ -719,6 +720,12 @@
   if (!CodeGenOpts.RewriteMapFiles.empty())
 addSymbolRewriterPass(CodeGenOpts, );
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage symbols
+  // with unique names.
+  if (CodeGenOpts.UniqueInternalLinkageNames) {
+MPM.add(createUniqueInternalLinkageNamesPass());
+  }
+
   if (Optional Options = getGCOVOptions(CodeGenOpts)) {
 MPM.add(createGCOVProfilerPass(*Options));
 if (CodeGenOpts.getDebugInfo() == codegenoptions::NoDebugInfo)
@@ -1203,6 +1210,12 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (CodeGenOpts.UniqueInternalLinkageNames) {
+MPM.addPass(UniqueInternalLinkageNamesPass());
+  }
+
   // Lastly, add semantically necessary passes for LTO.
   if (IsLTO || IsThinLTO) {
 MPM.addPass(CanonicalizeAliasesPass());
@@ -1292,6 +1305,12 @@
   MPM.addPass(InstrProfiling(*Options, false));
 });
 
+  // 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 262791.
tmsriram added a comment.

Rebase.


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,61 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// 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 i32 @_ZL4mverv()
+// PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -957,6 +957,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,27 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-f[no]-unique-internal-linkage-names**
+
+   Controls whether Clang emits a unique (best-effort) symbol name for internal
+   linkage symbols.  When this option is set, compiler hashes the main source
+   file path from the command line and appends it to all internal symbols. If a
+   program contains multiple objects compiled with the same command-line source
+   file path, the symbols are not guaranteed to be unique.  This option is
+   particularly useful in attributing profile information to the correct
+   function when multiple functions with the same private linkage name exist
+   in the binary.
+
+   It should be noted that this option cannot guarantee uniqueness and the
+   following is an example where it is not unique when two modules contain
+   symbols with the same private linkage name:
+
+   .. code-block:: 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

@rsmith Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

@rnk Ping.


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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:486-501
+  Options.BBSections =
+  llvm::StringSwitch(CodeGenOpts.BBSections)
+  .Case("all", llvm::BasicBlockSection::All)
+  .Case("labels", llvm::BasicBlockSection::Labels)
+  .Case("none", llvm::BasicBlockSection::None)
+  .Default(llvm::BasicBlockSection::List);
+

rsmith wrote:
> I don't like doing the parsing here. But... this is consistent with what the 
> other options above do, so I'm OK with keep it like this for consistency. (If 
> someone wants to clean this up we can do that separately for all these enum 
> options.)
Acknowledged.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 259626.
tmsriram marked 5 inline comments as done.
tmsriram added a comment.
Herald added subscribers: dexonsmith, steven_wu, hiraditya, arichardson, emaste.
Herald added a reviewer: espindola.

Document the flags, rename the options.


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

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basic-block-sections.c
  clang/test/CodeGen/basic-block-sections.funcnames
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,16 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -167,7 +167,7 @@
   bool ltoCSProfileGenerate;
   bool ltoDebugPassManager;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1975
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

rsmith wrote:
> It's not great to use the same argument as both one of three specific strings 
> and as an arbitrary filename. This not only prevents using those three names 
> as the file name, it also means adding any more specific strings is a 
> backwards-incompatible change. Can you find some way to tweak this to avoid 
> that problem?
Understood, I have the following suggestions:

1) Would this be ugly?  -fbasicblock-sections=list=
2) I could make this a completely new option.

Is there a preference?  Thanks.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 3 inline comments as done.
tmsriram added a comment.

@rnk Good now?


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D73307#1989924 , @erichkeane wrote:

> In D73307#1989740 , @rnk wrote:
>
> > In D73307#1978140 , @tmsriram 
> > wrote:
> >
> > > In D73307#1972388 , @rnk wrote:
> > >
> > > > Regarding the alias attribute, it occurs to me that reimplementing this 
> > > > as an early LLVM pass would not have that problem. Do you think that 
> > > > would be worth doing?
> > >
> > >
> > > I can try doing this.  If my understanding is right,  you are suggesting 
> > > that doing this later would mean the alias would have already been 
> > > applied?.  However, for multi-versioning symbols, I must parse the target 
> > > name to insert the module name in between right?
> >
> >
> >
>




>> Yes, Clang will apply the alias, do all name mangling, etc, and then 
>> renaming will happen later.
>> 
>> It's not clear to me if the order of the `.` suffix on 
>> multi-versioned symbols matters. @erichkeane, should it? Supposing some 
>> mid-level optimization came along and did function versioning, it would 
>> rename the internal function and append `.1`. So, I think appending as you 
>> implemented is fine.
>> 
>>  ---
>> 
>> All of my concerns have been addressed, and I think everyone else's are as 
>> well. We have documentation indicating what we mean by uniqueness which 
>> @hubert.reinterpretcast wanted clarified, we had the prefix map issue which 
>> I believe is addressed, so to my knowledge this is ready.
>> 
>> Let's wait a bit to hear from Erich, but otherwise I think this is ready in 
>> a few days.
> 
> Multiversioning doesn't care about the names, simply that they are 
> unique/unique from eachother.  The suffix that GCC/ICC put together for these 
> two types is simply to make sure we don't get something that conflicts with 
> another symbol.  As long as these symbols are unique (and correclty updated!) 
> , they can be named absolutely anything.



In D73307#1989924 , @erichkeane wrote:

> In D73307#1989740 , @rnk wrote:
>
> > In D73307#1978140 , @tmsriram 
> > wrote:
> >
> > > In D73307#1972388 , @rnk wrote:
> > >
> > > > Regarding the alias attribute, it occurs to me that reimplementing this 
> > > > as an early LLVM pass would not have that problem. Do you think that 
> > > > would be worth doing?
> > >
> > >
> > > I can try doing this.  If my understanding is right,  you are suggesting 
> > > that doing this later would mean the alias would have already been 
> > > applied?.  However, for multi-versioning symbols, I must parse the target 
> > > name to insert the module name in between right?
> >
> >
> > Yes, Clang will apply the alias, do all name mangling, etc, and then 
> > renaming will happen later.
> >
> > It's not clear to me if the order of the `.` suffix on 
> > multi-versioned symbols matters. @erichkeane, should it? Supposing some 
> > mid-level optimization came along and did function versioning, it would 
> > rename the internal function and append `.1`. So, I think appending as you 
> > implemented is fine.
> >
> >  ---
> >
> > All of my concerns have been addressed, and I think everyone else's are as 
> > well. We have documentation indicating what we mean by uniqueness which 
> > @hubert.reinterpretcast wanted clarified, we had the prefix map issue which 
> > I believe is addressed, so to my knowledge this is ready.
> >
> > Let's wait a bit to hear from Erich, but otherwise I think this is ready in 
> > a few days.
>
>
> Multiversioning doesn't care about the names, simply that they are 
> unique/unique from eachother.  The suffix that GCC/ICC put together for these 
> two types is simply to make sure we don't get something that conflicts with 
> another symbol.  As long as these symbols are unique (and correclty updated!) 
> , they can be named absolutely anything.


Thanks for the comments!  I was one of the authors of the GCC multiversioning 
patch, and if I recall correctly from years ago, one of the reasons for naming 
the target clones of multi-versioned foo as foo, foo.sse, etc. was so that 
c++filt works nicely on this.  Like, c++filt on _Z3foov.see would demangle as 
foo() [clone sse].  But c++filt  is  broken IMO as it is not handling these 
suffixes correctly any more, like c++filt _Z3foov.sse4.2 does not even demangle 
as expected.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 257853.
tmsriram added a comment.

Updated this patch, using a pass to convert symbol names in D78243 


Also, removed the test for -fmacro-prefix-map.  For -ffile-prefix-map, looks 
like getSourceFileName() should be modified to contain the new path.


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,61 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// 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 i32 @_ZL4mverv()
+// PLAIN: define internal i32 @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -959,6 +959,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,27 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-f[no]-unique-internal-linkage-names**
+
+   Controls whether Clang emits a unique (best-effort) symbol name for internal
+   linkage symbols.  When this option is set, compiler hashes the main source
+   file path from the command line and appends it to all internal symbols. If a
+   program contains multiple objects compiled with the same command-line source
+   file path, the symbols are not guaranteed to be unique.  This option is
+   particularly useful in attributing profile information to the correct
+   function when multiple functions with the same private linkage name exist
+   in 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 2 inline comments as done.
tmsriram added a comment.

In D73307#1972388 , @rnk wrote:

> Regarding the alias attribute, it occurs to me that reimplementing this as an 
> early LLVM pass would not have that problem. Do you think that would be worth 
> doing?


I can try doing this.  If my understanding is right,  you are suggesting that 
doing this later would mean the alias would have already been applied?.  
However, for multi-versioning symbols, I must parse the target name to insert 
the module name in between right?




Comment at: clang/docs/UsersManual.rst:1684
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple

rnk wrote:
> tmsriram wrote:
> > hubert.reinterpretcast wrote:
> > > MaskRay wrote:
> > > > rnk wrote:
> > > > > I think it's important to be specific here. I'd go so far as to say:
> > > > > "When this option is set, compiler hashes the main source file path 
> > > > > from the command line and appends it to all internal symbols. If a 
> > > > > program contains multiple objects compiled from the same source file 
> > > > > path, the symbols are not guaranteed to be unique."
> > > > Should option like -ffile-prefix-map= affect the hashed source file 
> > > > path? I suspect it already does this but I haven't verified.
> > > > 
> > > > This may need tests for Windows and non-Windows, similar to D77223.
> > > With respect to @rnk's suggestion, "from the same source file path" 
> > > implies the same file. I would suggest saying "with the same command-line 
> > > source file specification".
> > I have handled this now and added a test.  I saw the reference but don't 
> > quite understand the need for windows test?  I am not familiar with this.
> Would the file prefix map not already have been applied when creating the 
> LLVM IR module source file path? I think it should be, so I'm not sure the 
> new code you added is in the right place.
Module.getSourceFileName() never changes.   I checked here and also at 
AsmPrinter.cpp.  I am not sure this a bug, maybe I should just fix that first.



Comment at: clang/lib/CodeGen/CodeGenModule.h:31
 #include "clang/Basic/XRayLists.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/DenseMap.h"

rnk wrote:
> Why add this include here? I don't see any reason it is needed. Maybe the cpp 
> file needs it?
Yes, the cpp file needs this to access the MacroPrefixMap.


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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1972297 , @dblaikie wrote:

> In D68049#1971276 , @MaskRay wrote:
>
> > In D68049#1970825 , @tmsriram 
> > wrote:
> >
> > > Ping.
> >
> >
> > @rsmith ^^^
> >
> > More specific question, do you think 
> > `clang/test/CodeGen/basicblock-sections.c` should be converted to a `-S 
> > -emit-llvm` test?
>
>
> My understanding is that because this patch only changes TargetOptions, it 
> has no effect on the emitted LLVM IR - only on how that IR is translated to 
> machine code. (like -ffunction-sections, for instance - that property is not 
> persisted in the LLVM IR, it's an API-level communication between Clang and 
> the LLVM backends) So either it goes untested, or it gets tested by a 
> source->assembly test.


Yes, David is right.  Machine IR after BBSectionsPrepare is the first point at 
which this option has an effect.  This is similar to function-sections.c.  I 
acknowledge the concern that tests should not do more than what is required.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

We just noticed an issue with alias attribute and this option.  Here is the 
code that exposes the problem:

alias_global.c

static int foo;
extern int bar __attribute__((alias("foo")))

$ clang -c alias_global.c -funique-internal-linkage-names
alias_global.c:4:31: error: alias must point to a defined variable or function
extern int bar __attribute__((alias("foo")));

This happens because bar is aliased to "foo" and not "foo.".

I would like to know your thoughts on this?  Here is what I think.  alias 
attribute is a bit special in that the symbol name must be given carefully, for 
instance the mangled name if it is a C++ compile and the actual name for a C 
compile.  Even without the option, the same error will happen if a C++ compiler 
was used on this code.  So, in some sense, alias attribute requires the user to 
know what symbol name to expect.  -funique-internal-linkage-names can be 
treated as just an additional extension of this where the user must append the 
md5 hash of the module, which can be computed independently  with:

$ echo -n alias_global.c | md5sum

So, I suggest we leave it as it is and add to the documentation that this needs 
to be kept in mind.


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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D68049



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/docs/UsersManual.rst:1684
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple

hubert.reinterpretcast wrote:
> MaskRay wrote:
> > rnk wrote:
> > > I think it's important to be specific here. I'd go so far as to say:
> > > "When this option is set, compiler hashes the main source file path from 
> > > the command line and appends it to all internal symbols. If a program 
> > > contains multiple objects compiled from the same source file path, the 
> > > symbols are not guaranteed to be unique."
> > Should option like -ffile-prefix-map= affect the hashed source file path? I 
> > suspect it already does this but I haven't verified.
> > 
> > This may need tests for Windows and non-Windows, similar to D77223.
> With respect to @rnk's suggestion, "from the same source file path" implies 
> the same file. I would suggest saying "with the same command-line source file 
> specification".
I have handled this now and added a test.  I saw the reference but don't quite 
understand the need for windows test?  I am not familiar with this.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 255871.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Change description and handle -ffile-prefix-map/-fmacro-prefix-map.


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/CodeGen/unique-internal-linkage-names2.c
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names2.c
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names2.c
@@ -0,0 +1,15 @@
+// This test checks if -funique-internal-linkage-names uses the right
+// path when -ffile-prefix-map= (-fmacro-prefix-map=) is enabled.
+// With -fmacro-prefix-map=%p=NEW, this file must be referenced as:
+// NEW/unique-internal-linkage-names2.c
+// MD5 hash of "NEW/unique-internal-linkage-names2.c" :
+// $ echo -n NEW/unique-internal-linkage-names2.c | md5sum
+// bd816b262f03c98ffb082cde0847804c
+// RUN: %clang_cc1 -triple x86_64 -funique-internal-linkage-names -fmacro-prefix-map=%p=NEW -x c -S -emit-llvm -o - %s | FileCheck %s
+static int glob;
+
+int getGlob() {
+  return glob;
+}
+
+// CHECK: glob.bd816b262f03c98ffb082cde0847804c = internal global
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,68 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// PLAIN: @_ZL4glob = internal global
+// PLAIN: @_ZL3foov()
+// PLAIN: @_ZN12_GLOBAL__N_14getMEv
+// PLAIN: @_ZZ8retAnonMvE5fGlob
+// PLAIN: @_ZN12_GLOBAL__N_16anon_mE
+// PLAIN: @_ZL4mverv.resolver()
+// PLAIN: @_ZL4mverv()
+// PLAIN: @_ZL4mverv.sse4.2()
+// UNIQUE-NOT: @_ZL4glob = internal global
+// UNIQUE-NOT: @_ZL3foov()
+// UNIQUE-NOT: @_ZN12_GLOBAL__N_14getMEv
+// UNIQUE-NOT: @_ZZ8retAnonMvE5fGlob
+// UNIQUE-NOT: @_ZN12_GLOBAL__N_16anon_mE
+// UNIQUE-NOT: @_ZL4mverv.resolver()
+// UNIQUE-NOT: @_ZL4mverv()
+// UNIQUE-NOT: @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}}
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}}
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.resolver()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.sse4.2()
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/test/CodeGen/basicblock-sections.c:35
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world

MaskRay wrote:
> I haven't read through the previous threads whether we should use a .c -> .s 
> test here. Assume we've decided to do that, `@progbits` should be followed by 
> `{{$}}` to ensure there is no unique assembly linkage.
Fixed.  About .c->.s, this is similar to what function-sections.c does too. 
Please let me know if I missed anything.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 254999.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Fix test and make error check at driver.


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

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/Driver/fbasicblock-sections.c
  clang/test/Driver/funique-bb-section-names.c

Index: clang/test/Driver/fbasicblock-sections.c
===
--- /dev/null
+++ clang/test/Driver/fbasicblock-sections.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -fbasicblock-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -fbasicblock-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -fbasicblock-sections=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -fbasicblock-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+//
+// CHECK-OPT-NONE: "-fbasicblock-sections=none"
+// CHECK-OPT-ALL: "-fbasicblock-sections=all"
+// CHECK-OPT-LIST: -fbasicblock-sections={{[^ ]*}}fbasicblock-sections.c
+// CHECK-OPT-LABELS: "-fbasicblock-sections=labels"
Index: clang/test/Driver/funique-bb-section-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-bb-section-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-bb-section-names -fno-unique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-bb-section-names"
+// CHECK-NOOPT-NOT: "-funique-bb-section-names"
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world:
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world:
+// BB_LABELS: a.BB.world:
+// BB_LABELS: aa.BB.world:
+// BB_LABELS: a.BB.another:
+//
+// BB_WORLD: .section .text.world,"ax",@progbits{{$}}
+// BB_WORLD: world:
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world:
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another:
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another:
+// BB_LIST-NOT: a.BB.another:
+//
+// UNIQUE: .section .text.world.a.BB.world,
+// UNIQUE: .section .text.another.a.BB.another,
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -954,10 +954,18 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 254347.
tmsriram added a comment.

Rebase and add tests for anonymous namespace symbol and function static global.


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,68 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Function local static variable and anonymous namespace namespace variable.
+namespace {
+int anon_m;
+int getM() {
+  return anon_m;
+}
+} // namespace
+
+int retAnonM() {
+  static int fGlob;
+  return getM() + fGlob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// PLAIN: @_ZL4glob = internal global
+// PLAIN: @_ZL3foov()
+// PLAIN: @_ZN12_GLOBAL__N_14getMEv
+// PLAIN: @_ZZ8retAnonMvE5fGlob
+// PLAIN: @_ZN12_GLOBAL__N_16anon_mE
+// PLAIN: @_ZL4mverv.resolver()
+// PLAIN: @_ZL4mverv()
+// PLAIN: @_ZL4mverv.sse4.2()
+// UNIQUE-NOT: @_ZL4glob = internal global
+// UNIQUE-NOT: @_ZL3foov()
+// UNIQUE-NOT: @_ZN12_GLOBAL__N_14getMEv
+// UNIQUE-NOT: @_ZZ8retAnonMvE5fGlob
+// UNIQUE-NOT: @_ZN12_GLOBAL__N_16anon_mE
+// UNIQUE-NOT: @_ZL4mverv.resolver()
+// UNIQUE-NOT: @_ZL4mverv()
+// UNIQUE-NOT: @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}}
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}}
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.resolver()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.sse4.2()
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,24 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-f[no]-unique-internal-linkage-names**
+
+   Controls whether Clang emits a unique (best-effort) symbol name for internal
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple
+   functions with the same private linkage name exist in the binary.
+
+   It should be noted that this option cannot guarantee uniqueness and the
+   following is an example where it is not unique when two modules contain
+   symbols with the same private linkage name:
+
+   .. code-block:: console
+
+ $ cd $P/foo && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:5
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm 
-funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;

wmi wrote:
> MaskRay wrote:
> > Might be worth adding a few other internal linkage names.
> > 
> > * an object in an unnamed namespace
> > * an extern "C" static function
> > * a function-local static variable
> > * `label: &`
> > 
> > Hope @mtrofin and @davidxl can clarify what internal names may benefit AFDO 
> > and we can add such internal names specifically.
> Only internal functions matter for AFDO. Other types of internal names are 
> irrelevant.
extern "C" static is not very useful as the name is not exposed outside the 
object file.  C++ compiler will still mangle the name. for eg:

a.cpp

extern "C" {
  static int bar() {
return 0;
  }
}

int foo() {
  printf("%p\n", bar);
}

$ nm a.o 
0040 t _ZL3barv


Also, for label: &, it is not preserved in the symbol table as a .L 
is used which is deleted by the assembler.  I can throw a asm("") directive but 
that will bypass the front-end.  Is there anyway to preserve this?  

I can add the rest of the cases.




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

https://reviews.llvm.org/D73307



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 253271.
tmsriram marked 5 inline comments as done.
tmsriram added a reviewer: eli.friedman.
tmsriram added a comment.

Clang options for Basic Block Sections enabled in D68063 
 and D73674 

1. -fbasicblock-sections = { "all" | "" | "labels" | "none" }
2. -funique-bb-section-names

Added more tests.  Rebased and checked that it applies cleanly on trunk.


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

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/Driver/fbasicblock-sections.c
  clang/test/Driver/funique-bb-section-names.c

Index: clang/test/Driver/fbasicblock-sections.c
===
--- /dev/null
+++ clang/test/Driver/fbasicblock-sections.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -fbasicblock-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -fbasicblock-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -fbasicblock-sections=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -fbasicblock-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+//
+// CHECK-OPT-NONE: "-fbasicblock-sections=none"
+// CHECK-OPT-ALL: "-fbasicblock-sections=all"
+// CHECK-OPT-LIST: -fbasicblock-sections={{[^ ]*}}fbasicblock-sections.c
+// CHECK-OPT-LABELS: "-fbasicblock-sections=labels"
Index: clang/test/Driver/funique-bb-section-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-bb-section-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-bb-section-names -fno-unique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-bb-section-names"
+// CHECK-NOOPT-NOT: "-funique-bb-section-names"
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -953,11 +953,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D73307#1942805 , @MaskRay wrote:

> In D73307#1932131 , @rnk wrote:
>
> > At a higher level, should this just be an IR pass that clang adds into the 
> > pipeline when the flag is set? It should be safe to rename internal 
> > functions and give private functions internal linkage. It would be less 
> > invasive to clang and have better separation of concerns. As written, this 
> > is based on the source filename on the module, which is accessible from IR. 
> > The only reason I can think of against this is that the debug info might 
> > refer to the function linkage name, but maybe that is calculated later 
> > during codegen.
>
>
> I second this suggestion. `clang/lib/CodeGen/BackendUtil.cpp` 
> `EmitAssemblyHelper::EmitAssemblyWithNewPassManager` ModulePassManager may be 
> a more appropriate place for this feature. There are examples from both sides:


@rnk getMangledNameImpl for example adds multi-versioning suffixes,  
"__regcall3_" and  "__device_stub".  Adding the hash is just so simple.  Is a 
pass really needed?  If so, I should parse "." suffixed 
Multi-versioining names and insert the hash in between as the demangling looks 
better?

> 
> 
> - clang frontend: `#pragma redefine_extname`
> - middle end IR->IR: the old pass manager has a feature -frewrite-map-file, 
> which does a similar thing but is implemented as an IR transformation.






Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1060
+  const Decl *D = GD.getDecl();
+  if (CGM.getCodeGenOpts().UniqueInternalLinkageNames &&
+  !CGM.getModuleNameHash().empty() &&

davidxl wrote:
> is the first check redundant now?
Good catch, converted to an assert.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 252688.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Changes to description of flag, remove redundant check.


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,46 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// PLAIN: @_ZL4glob = internal global
+// PLAIN: @_ZL3foov()
+// PLAIN: @_ZL4mverv.resolver()
+// PLAIN: @_ZL4mverv()
+// PLAIN: @_ZL4mverv.sse4.2()
+// UNIQUE-NOT: @_ZL4glob = internal global
+// UNIQUE-NOT: @_ZL3foov()
+// UNIQUE-NOT: @_ZL4mverv.resolver()
+// UNIQUE-NOT: @_ZL4mverv()
+// UNIQUE-NOT: @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.resolver()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.sse4.2()
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,8 @@
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
 
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,24 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-f[no]-unique-internal-linkage-names**
+
+   Controls whether Clang emits a unique (best-effort) symbol name for internal
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple
+   functions with the same private linkage name exist in the binary.
+
+   It should be noted that this option cannot guarantee uniqueness and the
+   following is an example where it is not unique when two modules contain
+   symbols with the same private linkage name:
+
+   .. code-block:: console
+
+ $ cd $P/foo && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
+
 Profile Guided Optimization
 ---
 
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -307,6 +307,7 @@
   const TargetInfo 
   std::unique_ptr ABI;
   llvm::LLVMContext 
+  std::string ModuleNameHash = "";
 
   std::unique_ptr TBAA;
 
@@ -578,6 +579,8 @@
   /// Return true iff an Objective-C runtime has been configured.
   bool hasObjCRuntime() { return 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1125
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 

rnk wrote:
> There are several other callers of getMangledNameImpl. Should they also use 
> this suffix? They mostly have to do with function multiversioning. Those seem 
> like they could be internal and need this treatment.
I moved the suffix append to getMangledNameImpl.  Other mangled name 
manipulations are also done here.  Added tests for multiversioned internal 
linkage symbols.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;

hubert.reinterpretcast wrote:
> tmsriram wrote:
> > hubert.reinterpretcast wrote:
> > > davidxl wrote:
> > > > rnk wrote:
> > > > > davidxl wrote:
> > > > > > Source filenames are not guaranteed to be unique, or it does 
> > > > > > contain the path as well?
> > > > > It appears to contain the path as the compiler receives it on the 
> > > > > command line. However, it is possible to design a build where this 
> > > > > string is not unique:
> > > > > ```
> > > > > cd foo
> > > > > clang -c name_conflict.c 
> > > > > cd ../bar
> > > > > clang -c name_conflict.c
> > > > > clang foo/name_conflict.o bar/name_conflict.o
> > > > > ```
> > > > > 
> > > > > I don't think we should try to handle this case, but we should 
> > > > > document the limitation somewhere. Some build systems do operate 
> > > > > changing the working directory as they go.
> > > > > 
> > > > > Can you add some to the clang/docs/UsersManual.rst file? I'd expect 
> > > > > it to show up here:
> > > > > https://clang.llvm.org/docs/UsersManual.html#command-line-options
> > > > > 
> > > > > You can elaborate that the unique names are calculated using the 
> > > > > source path passed to the compiler, and in a build system where that 
> > > > > is not unique, the function names may not be unique.
> > > > > 
> > > > > ---
> > > > > 
> > > > > I have also used this construct in the past for building single-file 
> > > > > ABI compatibility test cases:
> > > > > 
> > > > > ```
> > > > > // foo.cpp
> > > > > #ifdef PART1
> > > > > // ...
> > > > > #elif defined(PART2)
> > > > > // ...
> > > > > #endif
> > > > > 
> > > > > $ cc -c foo.cpp -DPART1
> > > > > $ cc -c foo.cpp -DPART2
> > > > > ```
> > > > yes, the first example was the scenario I meant. I agree name conflict 
> > > > due that case should be really rare. If yes, we can always use content 
> > > > based uniqueid -- by appending llvm::getUniqueModuleId -- but that is 
> > > > probably overkill.
> > > > yes, the first example was the scenario I meant. I agree name conflict 
> > > > due that case should be really rare.
> > > Except for projects where it is the rule and not the exception. One 
> > > pattern where this occurs is when each subdirectory implements one class 
> > > and there are multiple classes that implement the same interface. In each 
> > > directory, the implementation of each class is separated into files by 
> > > grouping methods of the class. The set of filenames in one such directory 
> > > may well be the same as the set in another.
> > I am not sure much can be done here other than try using getUniqueModuleId. 
> >  Just to be clear, even if the file names are the same, the actual conflict 
> > happens only when the "make " is done from the leaf directory.  Doing:
> > 
> > cc a/name_conflict.cpp -c
> > cc b/name_conflict.cpp -c 
> > 
> > should still produce unique names.
> Understood. I do happen to have been working on a project that does the whole 
> `$(MAKE) -C a/` thing with such a project layout. Content based hashing is 
> unfortunate for other reasons: The hash would be unstable during the course 
> of maintenance on the codebase. I guess the limitation is okay for the 
> intended use case of the patch.
Acknowledged.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 251868.
tmsriram marked 11 inline comments as done.
tmsriram added a comment.

Address @rnk comments:

- Moved implementation to getMangledNameImpl to catch multi-versioned symbols 
and internal linkage globals
- Moved hash computation to consrtuctor
- Renamed option to -funique-internal-linkage-names
- Add documentation to UsersManual.rst and verified with html conversion
- Added tests for MV symbols and static globals, is there anything else? I am 
not too familiar with internal/private symbol possibilites


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

https://reviews.llvm.org/D73307

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  clang/test/Driver/funique-internal-linkage-names.c

Index: clang/test/Driver/funique-internal-linkage-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-linkage-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-linkage-names -fno-unique-internal-linkage-names %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-linkage-names"
+// CHECK-NOOPT-NOT: "-funique-internal-linkage-names"
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -0,0 +1,46 @@
+// This test checks if internal linkage symbols get unique names with
+// -funique-internal-linkage-names option.
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int glob;
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+int getGlob() {
+  return glob;
+}
+
+// Multiversioning symbols
+__attribute__((target("default"))) static int mver() {
+  return 0;
+}
+
+__attribute__((target("sse4.2"))) static int mver() {
+  return 1;
+}
+
+int mver_call() {
+  return mver();
+}
+
+// PLAIN: @_ZL4glob = internal global
+// PLAIN: @_ZL3foov()
+// PLAIN: @_ZL4mverv.resolver()
+// PLAIN: @_ZL4mverv()
+// PLAIN: @_ZL4mverv.sse4.2()
+// UNIQUE-NOT: @_ZL4glob = internal global
+// UNIQUE-NOT: @_ZL3foov()
+// UNIQUE-NOT: @_ZL4mverv.resolver()
+// UNIQUE-NOT: @_ZL4mverv()
+// UNIQUE-NOT: @_ZL4mverv.sse4.2()
+// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.resolver()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUE: @_ZL4mverv.{{[0-9a-f]+}}.sse4.2()
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,8 @@
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
 
+  Opts.UniqueInternalLinkageNames =
+  Args.hasArg(OPT_funique_internal_linkage_names);
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,23 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-f[no]-unique-internal-linkage-names**
+
+   Controls whether Clang emits a unique (best-effort) symbol name for internal
+   linkage symbols. The unique name is obtained by appending the MD5 hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple
+   functions with the same private linkage name exist in the binary.
+
+   It should be noted that this option cannot guarantee uniqueness and the
+   following is an example where it is not unique:
+
+   .. code-block:: console
+
+ $ cd $P/foo && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
+ $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
+
 Profile Guided Optimization
 ---
 
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 4 inline comments as done.
tmsriram added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;

hubert.reinterpretcast wrote:
> davidxl wrote:
> > rnk wrote:
> > > davidxl wrote:
> > > > Source filenames are not guaranteed to be unique, or it does contain 
> > > > the path as well?
> > > It appears to contain the path as the compiler receives it on the command 
> > > line. However, it is possible to design a build where this string is not 
> > > unique:
> > > ```
> > > cd foo
> > > clang -c name_conflict.c 
> > > cd ../bar
> > > clang -c name_conflict.c
> > > clang foo/name_conflict.o bar/name_conflict.o
> > > ```
> > > 
> > > I don't think we should try to handle this case, but we should document 
> > > the limitation somewhere. Some build systems do operate changing the 
> > > working directory as they go.
> > > 
> > > Can you add some to the clang/docs/UsersManual.rst file? I'd expect it to 
> > > show up here:
> > > https://clang.llvm.org/docs/UsersManual.html#command-line-options
> > > 
> > > You can elaborate that the unique names are calculated using the source 
> > > path passed to the compiler, and in a build system where that is not 
> > > unique, the function names may not be unique.
> > > 
> > > ---
> > > 
> > > I have also used this construct in the past for building single-file ABI 
> > > compatibility test cases:
> > > 
> > > ```
> > > // foo.cpp
> > > #ifdef PART1
> > > // ...
> > > #elif defined(PART2)
> > > // ...
> > > #endif
> > > 
> > > $ cc -c foo.cpp -DPART1
> > > $ cc -c foo.cpp -DPART2
> > > ```
> > yes, the first example was the scenario I meant. I agree name conflict due 
> > that case should be really rare. If yes, we can always use content based 
> > uniqueid -- by appending llvm::getUniqueModuleId -- but that is probably 
> > overkill.
> > yes, the first example was the scenario I meant. I agree name conflict due 
> > that case should be really rare.
> Except for projects where it is the rule and not the exception. One pattern 
> where this occurs is when each subdirectory implements one class and there 
> are multiple classes that implement the same interface. In each directory, 
> the implementation of each class is separated into files by grouping methods 
> of the class. The set of filenames in one such directory may well be the same 
> as the set in another.
I am not sure much can be done here other than try using getUniqueModuleId.  
Just to be clear, even if the file names are the same, the actual conflict 
happens only when the "make " is done from the leaf directory.  Doing:

cc a/name_conflict.cpp -c
cc b/name_conflict.cpp -c 

should still produce unique names.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 251469.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Address reviewer comments.

- reword comment
- rewrite test to use -emit-llvm


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-funcnames.c
  clang/test/Driver/funique-internal-funcnames.c

Index: clang/test/Driver/funique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-funcnames.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-funcnames -fno-unique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-funcnames"
+// CHECK-NOOPT-NOT: {{-funique-internal-funcnames}}
Index: clang/test/CodeGen/unique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-funcnames.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: @foo()
+// UNIQUE-NOT: @foo()
+// UNIQUE: @foo.{{[0-9a-f]+}}()
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,7 @@
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
 
+  Opts.UniqueInternalFuncNames = Args.hasArg(OPT_funique_internal_funcnames);
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1124,6 +1124,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use the hash of module name to get a unique
+  // identifier as this is a best-effort solution.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = ("." + Str).str();
+MangledName += UniqueSuffix;
+  }
+
   // Ensure either we have different ABIs between host and device compilations,
   // says host compilation following MSVC ABI but device compilation follows
   // Itanium C++ ABI or, if they follow the same ABI, kernel names after
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4222,6 +4222,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4819,6 +4821,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1971,6 +1971,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/test/CodeGen/unique-internal-funcnames.c:16
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.{{[0-9a-f]+}}:

lebedev.ri wrote:
> What does `getModule().getSourceFileName()` contain?
> The full path to the source file, or just the source file basename?
> If latter, maybe check the full name?
It contains the full path. 


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/test/CodeGen/unique-internal-funcnames.c:3
+
+// RUN: %clang -target x86_64 -S -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64 -S -funique-internal-funcnames -o -  %s | 
FileCheck %s --check-prefix=UNIQUE

davidxl wrote:
> MaskRay wrote:
> > You can hardly find any .c -> .s test in clang/test. We mostly do .c -> .ll 
> > testing. `.ll` -> `.s` are in llvm/test/CodeGen. 
> Is this convention documented somewhere? Any concerns of producing .s?
There are more than 200 tests in clang that do -S from .c -> .s and tens of 
tests in CodeGen alone.  I can change this to .ll but curious where this 
"hardly any" comes from?


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 251409.
tmsriram marked 2 inline comments as done.
tmsriram added a comment.

Address reviewer comments.  Fix test and delete blank line.


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-funcnames.c
  clang/test/Driver/funique-internal-funcnames.c

Index: clang/test/Driver/funique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-funcnames.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-funcnames -fno-unique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-internal-funcnames"
+// CHECK-NOOPT-NOT: {{-funique-internal-funcnames}}
Index: clang/test/CodeGen/unique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-funcnames.c
@@ -0,0 +1,16 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang -target x86_64 -S -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64 -S -funique-internal-funcnames -o -  %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.{{[0-9a-f]+}}:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -958,6 +958,7 @@
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);

+  Opts.UniqueInternalFuncNames = Args.hasArg(OPT_funique_internal_funcnames);
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1124,6 +1124,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use the hash of module name to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = ("." + Str).str();
+MangledName += UniqueSuffix;
+  }
+
   // Ensure either we have different ABIs between host and device compilations,
   // says host compilation following MSVC ABI but device compilation follows
   // Itanium C++ ABI or, if they follow the same ABI, kernel names after
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4222,6 +4222,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4819,6 +4821,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1971,6 +1971,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());

mtrofin wrote:
> Just a thought: md5 is a non-bijective transformation, afaik. How about using 
> base64 encoding, with the caveat that we replace + with $ and / with _ (so it 
> results in a valid name), and discard padding =
> 
> The value being, one can copy/paste that identifier, do the trivial 
> conversion back to base64 ($->+, _->/) and get the module name. Useful when 
> debugging, for example.
I dont think getting the module name back from the hash is super useful given 
that we can always inspect this symbol in gdb for source info.  


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 251220.
tmsriram marked 5 inline comments as done.
tmsriram added a comment.

Address Reviewer comments:

- Add new driver test.
- Fix existing test to only check for code output.


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique-internal-funcnames.c
  clang/test/Driver/funique-internal-funcnames.c

Index: clang/test/Driver/funique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/Driver/funique-internal-funcnames.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-internal-funcnames -fno-unique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: {{-funique-internal-funcnames}}
+// CHECK-NOOPT-NOT: {{-funique-internal-funcnames}}
Index: clang/test/CodeGen/unique-internal-funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique-internal-funcnames.c
@@ -0,0 +1,16 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang -target x86_64 -S -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64 -S -funique-internal-funcnames -o -  %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.{{[0-9a-f]+}}:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -957,6 +957,8 @@
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueInternalFuncNames = Args.hasArg(OPT_funique_internal_funcnames);
+
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1124,6 +1124,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use the hash of module name to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = ("." + Str).str();
+MangledName += UniqueSuffix;
+  }
+
   // Ensure either we have different ABIs between host and device compilations,
   // says host compilation following MSVC ABI but device compilation follows
   // Itanium C++ ABI or, if they follow the same ABI, kernel names after
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4222,6 +4222,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4819,6 +4821,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1971,6 +1971,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

>>> I think the patch series should probably be structured this way:
>>> 
>>> 1. LLVM CodeGen: enables basic block sections.
>>> 2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
>>> 3. LLVM CodeGen: which enables the rest of Propeller options.
>>> 4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of 
>>> Propeller features. It should not do hacky diassembly tasks.
>>> 5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4
>>> 
>>>   Making 1 and 2 separate can help move forward the patch series. 1 and 2 
>>> should not reference `llvm::propeller`.

It is now structured like how you mentioned above.  1) corresponds to D68063 
 and D73674 . 
 2) corresponds to this patch, D68049 .   
These patches do not reference "propeller" anywhere and are only about basic 
block sections and labels.

We will address Eli's comments in the LLVM patches.  We will also make 3), 4) 
and 5) like how you wanted.  Further, D68065  
is about LLD support for basic block sections and will not reference 
"propeller".


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1870094 , @tmsriram wrote:

> In D68049#1868623 , @MaskRay wrote:
>
> > > In D68049#1865967 , @MaskRay 
> > > wrote:
> > >  If you don't mind, I can push a Diff to this Differential which will 
> > > address these review comments.
> >
> > I can't because I can't figure out the patch relationship...
> >
> > First, this patch does not build on its own. I try applying D68063 
> >  first, then this patch. It still does not 
> > compile..
>


This should work now.  Please apply D68063  
first and then this one. Thanks!

> Weird, getBBSectionsList is defined by D68063 
> .  Let me try doing that again.  I will also 
> address the rest of your comments.
> 
>> 
>> 
>>   clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 
>> 'propeller' in namespace 'llvm'
>>   llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,
>> 
>> 
>> Chatted with shenhan and xur offline. I tend to agree that 
>> -fbasicblock-sections=label can improve profile accuracy. It'd be nice to 
>> make that feature separate,
>>  even if there is still a debate on whether the rest of Propeller is done in 
>> a maintainable way.
>> 
>> I think the patch series should probably be structured this way:
>> 
>> 1. LLVM CodeGen: enables basic block sections.
>> 2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
>> 3. LLVM CodeGen: which enables the rest of Propeller options.
>> 4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of 
>> Propeller features. It should not do hacky diassembly tasks.
>> 5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4
>> 
>>   Making 1 and 2 separate can help move forward the patch series. 1 and 2 
>> should not reference `llvm::propeller`.




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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 243947.
tmsriram added a comment.

Remove usage of "propeller". Fix header inclusion.


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

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -962,10 +962,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueBBSectionNames = Args.hasArg(OPT_funique_bb_section_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4202,8 +4202,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+options::OPT_fno_unique_bb_section_names,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4758,6 +4761,12 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
+
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fbasicblock-sections=") + A->getValue()));
+  }
+
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
UseSeparateSections)) {
 CmdArgs.push_back("-fdata-sections");
@@ -4767,6 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1870094 , @tmsriram wrote:

> In D68049#1868623 , @MaskRay wrote:
>
> > > In D68049#1865967 , @MaskRay 
> > > wrote:
> > >  If you don't mind, I can push a Diff to this Differential which will 
> > > address these review comments.
> >
> > I can't because I can't figure out the patch relationship...
> >
> > First, this patch does not build on its own. I try applying D68063 
> >  first, then this patch. It still does not 
> > compile..
>


Darn, the include of "llvm/ProfileData/BBSectionsProf.h" is missing in 
BackendUtil.cpp.  Made this mistake when splitting the patch. I will fix this 
shortly.

> Weird, getBBSectionsList is defined by D68063 
> .  Let me try doing that again.  I will also 
> address the rest of your comments.
> 
>> 
>> 
>>   clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 
>> 'propeller' in namespace 'llvm'
>>   llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,
>> 
>> 
>> Chatted with shenhan and xur offline. I tend to agree that 
>> -fbasicblock-sections=label can improve profile accuracy. It'd be nice to 
>> make that feature separate,
>>  even if there is still a debate on whether the rest of Propeller is done in 
>> a maintainable way.
>> 
>> I think the patch series should probably be structured this way:
>> 
>> 1. LLVM CodeGen: enables basic block sections.
>> 2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
>> 3. LLVM CodeGen: which enables the rest of Propeller options.
>> 4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of 
>> Propeller features. It should not do hacky diassembly tasks.
>> 5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4
>> 
>>   Making 1 and 2 separate can help move forward the patch series. 1 and 2 
>> should not reference `llvm::propeller`.




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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1868623 , @MaskRay wrote:

> > In D68049#1865967 , @MaskRay wrote:
> >  If you don't mind, I can push a Diff to this Differential which will 
> > address these review comments.
>
> I can't because I can't figure out the patch relationship...
>
> First, this patch does not build on its own. I try applying D68063 
>  first, then this patch. It still does not 
> compile..


Weird, getBBSectionsList is defined by D68063 
.  Let me try doing that again.  I will also 
address the rest of your comments.

> 
> 
>   clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 
> 'propeller' in namespace 'llvm'
>   llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,
> 
> 
> Chatted with shenhan and xur offline. I tend to agree that 
> -fbasicblock-sections=label can improve profile accuracy. It'd be nice to 
> make that feature separate,
>  even if there is still a debate on whether the rest of Propeller is done in 
> a maintainable way.
> 
> I think the patch series should probably be structured this way:
> 
> 1. LLVM CodeGen: enables basic block sections.
> 2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
> 3. LLVM CodeGen: which enables the rest of Propeller options.
> 4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of 
> Propeller features. It should not do hacky diassembly tasks.
> 5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4
> 
>   Making 1 and 2 separate can help move forward the patch series. 1 and 2 
> should not reference `llvm::propeller`.






Comment at: clang/lib/CodeGen/BackendUtil.cpp:444
+  while ((std::getline(fin, line)).good()) {
+StringRef S(line);
+// Lines beginning with @, # are not useful here.

grimar wrote:
> Something is wrong with the namings (I am not an expert in lib/CodeGen), but 
> you are mixing lower vs upper case styles: "fin", "line", "S", "R". Seems the 
> code around prefers upper case.
I am moving this function out of clang into llvm as this needs to be shared by 
llc, llvm and lld.  I will address all your comments for this function in the 
llvm change.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 243614.
tmsriram marked 3 inline comments as done.
tmsriram added a comment.

Removed getBBSectionsList (moved to LLVM) and address other reviewer comments.


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

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -962,10 +962,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueBBSectionNames = Args.hasArg(OPT_funique_bb_section_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4202,8 +4202,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+options::OPT_fno_unique_bb_section_names,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4758,6 +4761,11 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fbasicblock-sections=") + A->getValue()));
+  }
+
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1865967 , @MaskRay wrote:

> If you don't mind, I can push a Diff to this Differential which will address 
> these review comments.


Let me update this patch asap as we refactored getBBSectionsList into llvm as 
it is shared by llc, clang and lld.


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

https://reviews.llvm.org/D68049



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

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 242196.
tmsriram added a comment.

Splitting the clang patch into several pieces:

1. This is the parent patch and just contains support for basic block section 
options.
2. A separate patch for unique internal function names
3. A separate patch for an option to relocate with symbols instead of sections
4. A separate patch for Propeller flags for clang


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

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -955,14 +955,27 @@
   Opts.TrapFuncName = Args.getLastArgValue(OPT_ftrap_function_EQ);
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasFlag(OPT_ffunction_sections,
-   OPT_fno_function_sections, false);
+  Opts.BBSections = Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none");
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasFlag(OPT_ffunction_sections, OPT_fno_function_sections, false) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasFlag(OPT_fdata_sections,
OPT_fno_data_sections, false);
   Opts.StackSizeSection =
   Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
+  Opts.UniqueBBSectionNames = Args.hasFlag(
+  OPT_funique_bb_section_names, OPT_fno_unique_bb_section_names, false);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,8 +4153,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 240312.
tmsriram added a comment.

Minor changes.  Remove CC1option on "-fno", Remove "$" from unique suffix.


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique_internal_funcnames.c

Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang -target x86_64-pc-linux-gnu -S -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64-pc-linux-gnu -S -funique-internal-funcnames -o -  %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.{{[0-9a-f]+}}:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -964,6 +964,8 @@
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
 
+  Opts.UniqueInternalFuncNames = Args.hasArg(OPT_funique_internal_funcnames);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,6 +4153,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4695,6 +4697,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1101,6 +1101,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use "getUniqueModuleId" to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = ("." + Str).str();
+MangledName += UniqueSuffix;
+  }
+
   // Adjust kernel stub mangling as we may need to be able to differentiate
   // them from the kernel itself (e.g., for HIP).
   if (auto *FD = dyn_cast(GD.getDecl()))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1952,6 +1952,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], "funique-internal-funcnames">,
+  Group, Flags<[CC1Option]>,
+  HelpText<"Uniqueify Internal Linkage Function Names">;
+def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">,
+  Group;
+
 def fstrict_return : Flag<["-"], "fstrict-return">, Group,
   Flags<[CC1Option]>,
   HelpText<"Always treat control flow paths that fall off the end of a "
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1117
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = (".$" + Str).str();
+MangledName = MangledName + UniqueSuffix;

pcc wrote:
> Why `".$"` and not just `"."`?
I just wanted to keep it consistent with what getUniqueModuleId does with the 
Md5 hash.  I can remove it.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 240253.
tmsriram added a comment.

Remove an unnecessary header file inclusion.


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique_internal_funcnames.c

Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.${{[0-9a-f]+}}:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -964,6 +964,9 @@
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
 
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+  OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, false);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,6 +4153,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4695,6 +4697,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1101,6 +1101,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use "getUniqueModuleId" to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = (".$" + Str).str();
+MangledName = MangledName + UniqueSuffix;
+  }
+
   // Adjust kernel stub mangling as we may need to be able to differentiate
   // them from the kernel itself (e.g., for HIP).
   if (auto *FD = dyn_cast(GD.getDecl()))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1952,6 +1952,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], "funique-internal-funcnames">,
+  Group, Flags<[CC1Option]>,
+  HelpText<"Uniqueify Internal Linkage Function Names">;
+def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">,
+  Group, Flags<[CC1Option]>;
+
 def fstrict_return : Flag<["-"], "fstrict-return">, Group,
   Flags<[CC1Option]>,
   HelpText<"Always treat control flow paths that fall off the end of a "
Index: 

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 240249.
tmsriram added a comment.

Use Hash of Module's source file name directly.


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

https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique_internal_funcnames.c

Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.${{[0-9a-f]+}}:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -964,6 +964,9 @@
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
 
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+  OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, false);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,6 +4153,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4695,6 +4697,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -61,6 +61,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/TimeProfiler.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -1101,6 +1102,23 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use "getUniqueModuleId" to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+  !getModule().getSourceFileName().empty()) {
+llvm::MD5 Md5;
+Md5.update(getModule().getSourceFileName());
+llvm::MD5::MD5Result R;
+Md5.final(R);
+SmallString<32> Str;
+llvm::MD5::stringifyResult(R, Str);
+std::string UniqueSuffix = (".$" + Str).str();
+MangledName = MangledName + UniqueSuffix;
+  }
+
   // Adjust kernel stub mangling as we may need to be able to differentiate
   // them from the kernel itself (e.g., for HIP).
   if (auto *FD = dyn_cast(GD.getDecl()))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1952,6 +1952,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], "funique-internal-funcnames">,
+  Group, Flags<[CC1Option]>,
+  HelpText<"Uniqueify Internal Linkage Function Names">;
+def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">,
+  

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:966
  OPT_fno_unique_section_names, true);
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+  OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, 
false);

MaskRay wrote:
> Just `Args.hasArg(OPT_funique_internal_funcnames)`.
> 
> `-fno-*` is handled in clangDriver. CC1 does not need to know `-fno-*` if it 
> defaults to false.
Are you suggesting I remove the -fno-* flag? The -fno-* is useful  if 
-funique-internal-funcnames came added to a Makefile ,for example, and this 
needs to be disabled.  This is similar to say -ffunction-sections.  Please 
clarify. Thanks.


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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
+std::string UniqueSuffix = getUniqueModuleId((), true);
+if (!UniqueSuffix.empty()) {

MaskRay wrote:
> `std::string llvm::getUniqueModuleId(Module *M) {`
> 
> What is the second parameter?
> 
> What is the rationale that InternalLinkage is picked here?
This change to LLVM, https://reviews.llvm.org/D73310, contains the 
modifications to getUniqueModuleId to use the Module Identifier also in the 
hash.  We found that when "-z muldefs" are used or when there are no globals 
defined in a module, a unique hash could not be generated.  Using the full 
module name allows for a unique identifier in these cases.  The second 
parameter basically allows use of module name and we did not force this  to 
keep the existing usage as it is.

Internal Linkage is picked because there could be multiple functions of the 
same name with internal linkage in the final binary.  So, associating sampled 
profile symbols to the right function name becomes a problem.  Further, if 
multiple functions of the same name are hot, their profiles are aggregated 
affecting the optimization itself.  So, a unique name for internal linkage 
functions will clearly disambiguate the profile information.





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

https://reviews.llvm.org/D73307



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

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram created this revision.
tmsriram added reviewers: MaskRay, mehdi_amini, davidxl, pcc, rnk.
tmsriram added a parent revision: D68049: Propeller:  Clang options for basic 
block sections .

This is a subset of the functionality in Propeller patches.

This patch adds a new clang option, -funique-internal-funcnames, to generate 
unique names for functions with internal linkage.  This uses getUniqueModuleID 
and is a best effort and is highly likely that it will generate a unique name.


https://reviews.llvm.org/D73307

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/unique_internal_funcnames.c

Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.${{[0-9a-f]+}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -963,6 +963,8 @@
   Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+  OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, false);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,6 +4153,8 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_funique_internal_funcnames,
+options::OPT_fno_unique_internal_funcnames,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
 options::OPT_mrestrict_it,
@@ -4695,6 +4697,10 @@
 options::OPT_fno_unique_section_names, true))
 CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+   options::OPT_fno_unique_internal_funcnames, false))
+CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -61,6 +61,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/TimeProfiler.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -1101,6 +1102,18 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use "getUniqueModuleId" to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+  dyn_cast(GD.getDecl()) &&
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
+std::string UniqueSuffix = getUniqueModuleId((), true);
+if (!UniqueSuffix.empty()) {
+  MangledName = MangledName + '.' + UniqueSuffix;
+}
+  }
+
   // Adjust kernel stub mangling as we may need to be able to differentiate
   // them from the kernel itself (e.g., for HIP).
   if (auto *FD = dyn_cast(GD.getDecl()))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1952,6 +1952,12 @@
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], "funique-internal-funcnames">,
+  Group, Flags<[CC1Option]>,