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

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

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

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

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

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

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Sorry for the delay. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307 ___ cfe-commits mailing

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

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

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

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

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

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

erichkeane added a comment. > 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

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

tmsriram added a comment. In D73307#1989924 , @erichkeane wrote: > In D73307#1989740 , @rnk wrote: > > > In D73307#1978140 , @tmsriram > > wrote: > > > > > In

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

erichkeane added a comment. In D73307#1989740 , @rnk wrote: > In D73307#1978140 , @tmsriram wrote: > > > In D73307#1972388 , @rnk wrote: > > > > > Regarding the alias

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

rnk added a subscriber: erichkeane. rnk added a comment. In D73307#1978140 , @tmsriram wrote: > In D73307#1972388 , @rnk wrote: > > > Regarding the alias attribute, it occurs to me that reimplementing this as > >

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

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

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

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

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

rnk added a comment. 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? Comment at: clang/docs/UsersManual.rst:1684 + linkage symbols. The unique name is

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

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:

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

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

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

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:

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

hubert.reinterpretcast 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

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

MaskRay 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

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

rnk added a comment. Sorry for the delay, thanks for the update. Code looks good, but I want the documentation to be specific. 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

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

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

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

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

davidxl added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:29 +int mver_call() { + return mver(); +} [a side note]: interesting, since when Clang had this target attribute based multi-versioning and function overloading ?

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

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 +

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

wmi 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; MaskRay

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

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1165 const auto *ND = cast(GD.getDecl()); + std::string MangledName = getMangledNameImpl(*this, GD, ND); Delete this unrelated cosmetic change. CHANGES SINCE LAST ACTION

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

MaskRay added a comment. In D73307#1942838 , @tmsriram wrote: > In D73307#1942805 , @MaskRay wrote: > > > In D73307#1932131 , @rnk wrote: > > > > > At a higher level, should

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

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?

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

MaskRay added a comment. 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

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

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:

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

davidxl added inline comments. Comment at: clang/docs/UsersManual.rst:1683 + 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

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

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

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.

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

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 -

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

hubert.reinterpretcast added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135 +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); +llvm::MD5::MD5Result R; tmsriram wrote: > hubert.reinterpretcast wrote: > > davidxl

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

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

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

MaskRay 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

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

hubert.reinterpretcast added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135 +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); +llvm::MD5::MD5Result R; davidxl wrote: > rnk wrote: > > davidxl wrote: > > > Source

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

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135 +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); +llvm::MD5::MD5Result R; rnk wrote: > davidxl wrote: > > Source filenames are not guaranteed to be

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

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

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

rnk added a comment. I guess my initial reaction is that it is disappointing that downstream consumers can't cope with non-unique symbol names (What is the point of having internal linkage if we can't rely on it?), but given @mtrofin's input about AFDO, this seems like it may be a practical

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

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

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

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

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

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1129 + // should get unique names. Use the hash of module name to get a unique + // identifier and this is a best effort. + if (getCodeGenOpts().UniqueInternalFuncNames &&

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

davidxl added a comment. As far as I understand, this is not a propeller specific patch, but it fixes a general problem in AFDO as well. Perhaps change the summary to reflect that? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307

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

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1129 + // should get unique names. Use the hash of module name to get a unique + // identifier and this is a best effort. + if (getCodeGenOpts().UniqueInternalFuncNames &&

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

MaskRay 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

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

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:

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

MaskRay added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:961 + Opts.UniqueInternalFuncNames = Args.hasArg(OPT_funique_internal_funcnames); + Delete excess empty line. CHANGES SINCE LAST ACTION

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

MaskRay added inline comments. Comment at: clang/test/Driver/funique-internal-funcnames.c:3 +// RUN: %clang -### -funique-internal-funcnames -fno-unique-internal-funcnames %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s +// CHECK-OPT: {{-funique-internal-funcnames}} +//

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

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

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

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/

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

mtrofin added a comment. In D73307#1839984 , @MaskRay wrote: > In D73307#1839880 , @mtrofin wrote: > > > In D73307#1839829 , @MaskRay wrote: > > > > > The code change seems

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

MaskRay added a comment. In D73307#1839880 , @mtrofin wrote: > In D73307#1839829 , @MaskRay wrote: > > > The code change seems fine, but the test requires some changes. I haven't > > followed Propeller

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

mtrofin added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp: + !getModule().getSourceFileName().empty()) { +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); Just a thought: md5 is a non-bijective transformation,

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

mtrofin added a comment. In D73307#1839829 , @MaskRay wrote: > The code change seems fine, but the test requires some changes. I haven't > followed Propeller development, but hope someone with profile experience can > confirm InternalLinkage is the only

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

MaskRay added a comment. The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever

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

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:

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

MaskRay added inline comments. Comment at: clang/include/clang/Driver/Options.td:1959 +def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">, + Group, Flags<[CC1Option]>; + `, Flags<[CC1Option]>` can be deleted.

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

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;

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

pcc added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1117 +llvm::MD5::stringifyResult(R, Str); +std::string UniqueSuffix = (".$" + Str).str(); +MangledName = MangledName + UniqueSuffix; Why `".$"` and not just `"."`? CHANGES

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

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

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

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

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

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

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

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

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

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

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

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,