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

[Hongtao Yu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG01f0d162d672: Moving UniqueInternalLinkageNamesPass to the 
start of IR pipelines. (authored by hoy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-pseudo-probe.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -134,6 +134,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -247,6 +254,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -282,6 +292,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,24 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/test/Other/new-pm-pseudo-probe.ll
===
--- /dev/null
+++ llvm/test/Other/new-pm-pseudo-probe.ll
@@ -0,0 +1,12 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+
+define void @foo() {
+  ret void
+}
+
+;; Check the SampleProfileProbePass is enabled under the -new-pm-pseudo-probe-for-profiling switch.
+;; The switch can be used to test a specific pass order in a particular setup, e.g, in unique-internal-linkage-names.ll
+; CHECK: Running pass: SampleProfileProbePass
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -285,6 +285,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfil

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314257.
hoy added a comment.

Addressing comments from dblaikie.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-pseudo-probe.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -134,6 +134,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -247,6 +254,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -282,6 +292,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,24 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/test/Other/new-pm-pseudo-probe.ll
===
--- /dev/null
+++ llvm/test/Other/new-pm-pseudo-probe.ll
@@ -0,0 +1,12 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+
+define void @foo() {
+  ret void
+}
+
+;; Check the SampleProfileProbePass is enabled under the -new-pm-pseudo-probe-for-profiling switch.
+;; The switch can be used to test a specific pass order in a particular setup, e.g, in unique-internal-linkage-names.ll
+; CHECK: Running pass: SampleProfileProbePass
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -285,6 +285,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
+  UniqueLinkageNames = false;
 }
 
 extern cl::opt Enable

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2475856 , @dblaikie wrote:

> Looks good - test cases might benefit from some descriptive comments 
> (explaining why the pseudo probe pass needs to be enabled to test the unique 
> linkage name pass - I guess to check that it appears in just the right place 
> in the pass pipeline?)

Comments added. Thanks for reviewing the patch.


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Looks good - test cases might benefit from some descriptive comments 
(explaining why the pseudo probe pass needs to be enabled to test the unique 
linkage name pass - I guess to check that it appears in just the right place in 
the pass pipeline?)


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314200.
hoy marked an inline comment as done.
hoy added a comment.

Adding a test for the new pseudo probe test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-pseudo-probe.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -133,6 +133,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -246,6 +253,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -281,6 +291,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,23 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/test/Other/new-pm-pseudo-probe.ll
===
--- /dev/null
+++ llvm/test/Other/new-pm-pseudo-probe.ll
@@ -0,0 +1,10 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -debug-pass-manager < %s 2>&1 | FileCheck %s
+
+define void @foo() {
+  ret void
+}
+
+; CHECK: Running pass: SampleProfileProbePass
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -284,6 +284,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
+  UniqueLinkageNames = false;
 }
 
 extern cl::opt EnableConstraintElimination;
@@ -1001,6 +1002,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique na

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

[Hongtao Yu via Phabricator via cfe-commits]

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



Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));

dblaikie wrote:
> aeubanks wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > I guess this should probably have some separate testing, if it's a 
> > > > separate flag/feature? (& flag+tests could be in a separate commit)
> > > I'm not sure there's a separate need for this switch except for being 
> > > tested in `unique-internal-linkage-names.ll`. The point of this whole 
> > > patch is to place the unique name pass before the pseudo probe pass and 
> > > test it works. Hence it sounds appropriate to me to include all changes 
> > > in one patch. What do you think?
> > +1 to hoy's comment. I don't think there's a need to make patches strictly 
> > as incremental as possible if they're already small. (I would have been 
> > okay with keeping the Clang change here FWIW)
> I understand I'm a bit of a stickler for some of this stuff - though the 
> particular reason I brought it up here is that this adds two flags, but 
> doesn't test them separately, only together. So it's not clear/tested as to 
> which behaviors are provided by which flags.
> 
> Separating the flags would make it clear that each flag/functionality was 
> tested fully.
> 
> Please add test coverage for each flag separately, optionally separate this 
> into two patches to make it clearer how each piece of functionality is tested.
Sounds good, a separate test added for the pseudo probe flag.


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));

aeubanks wrote:
> hoy wrote:
> > dblaikie wrote:
> > > I guess this should probably have some separate testing, if it's a 
> > > separate flag/feature? (& flag+tests could be in a separate commit)
> > I'm not sure there's a separate need for this switch except for being 
> > tested in `unique-internal-linkage-names.ll`. The point of this whole patch 
> > is to place the unique name pass before the pseudo probe pass and test it 
> > works. Hence it sounds appropriate to me to include all changes in one 
> > patch. What do you think?
> +1 to hoy's comment. I don't think there's a need to make patches strictly as 
> incremental as possible if they're already small. (I would have been okay 
> with keeping the Clang change here FWIW)
I understand I'm a bit of a stickler for some of this stuff - though the 
particular reason I brought it up here is that this adds two flags, but doesn't 
test them separately, only together. So it's not clear/tested as to which 
behaviors are provided by which flags.

Separating the flags would make it clear that each flag/functionality was 
tested fully.

Please add test coverage for each flag separately, optionally separate this 
into two patches to make it clearer how each piece of functionality is tested.



Comment at: llvm/tools/opt/NewPMDriver.cpp:253-258
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);

hoy wrote:
> dblaikie wrote:
> > Both of these might be more readably written as something like:
> > ```
> > P.emplace();
> > P.PseudoProbeForProfiling = true;
> > ```
> > & similar. (you can commit the change to DebugInfoForProfiling separately 
> > before/after this change to make it consistent with the new one)
> > 
> > But no big deal either way - while it makes these tidier it makes them a 
> > bit less consistent with the other three
> That looks cleaner, but there are assertions in the constructor of 
> `PGOOptions` which I would not like to bypass by setting the 
> `PseudoProbeForProfiling` field separately.
Ah, fair enough.

Though given the struct has publicly mutable members, these assertions aren't 
especially effective. Looks like they started being added here: 
https://reviews.llvm.org/D36040 - which I guess doesn't tell us much, but I was 
curious. Wonder if we could move the checks to somewhere more robust. (separate 
patch, in any case)


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.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));

hoy wrote:
> dblaikie wrote:
> > I guess this should probably have some separate testing, if it's a separate 
> > flag/feature? (& flag+tests could be in a separate commit)
> I'm not sure there's a separate need for this switch except for being tested 
> in `unique-internal-linkage-names.ll`. The point of this whole patch is to 
> place the unique name pass before the pseudo probe pass and test it works. 
> Hence it sounds appropriate to me to include all changes in one patch. What 
> do you think?
+1 to hoy's comment. I don't think there's a need to make patches strictly as 
incremental as possible if they're already small. (I would have been okay with 
keeping the Clang change here FWIW)


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314043.
hoy added a comment.

Removing the checks of VerifierAnalysis in test code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -133,6 +133,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -246,6 +253,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -281,6 +291,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,23 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -284,6 +284,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
+  UniqueLinkageNames = false;
 }
 
 extern cl::opt EnableConstraintElimination;
@@ -1001,6 +1002,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1770,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));

dblaikie wrote:
> I guess this should probably have some separate testing, if it's a separate 
> flag/feature? (& flag+tests could be in a separate commit)
I'm not sure there's a separate need for this switch except for being tested in 
`unique-internal-linkage-names.ll`. The point of this whole patch is to place 
the unique name pass before the pseudo probe pass and test it works. Hence it 
sounds appropriate to me to include all changes in one patch. What do you think?



Comment at: llvm/tools/opt/NewPMDriver.cpp:253-258
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);

dblaikie wrote:
> Both of these might be more readably written as something like:
> ```
> P.emplace();
> P.PseudoProbeForProfiling = true;
> ```
> & similar. (you can commit the change to DebugInfoForProfiling separately 
> before/after this change to make it consistent with the new one)
> 
> But no big deal either way - while it makes these tidier it makes them a bit 
> less consistent with the other three
That looks cleaner, but there are assertions in the constructor of `PGOOptions` 
which I would not like to bypass by setting the `PseudoProbeForProfiling` field 
separately.


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

hoy wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > aeubanks wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > aeubanks wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > Does this test validate the new behavior? (ie: does 
> > > > > > > > > > > > > this test fail without the LLVM changes and pass with 
> > > > > > > > > > > > > it) Not that it necessarily has to - since Clang 
> > > > > > > > > > > > > isn't here to test the LLVM behavior - perhaps this 
> > > > > > > > > > > > > test is sufficient in Clang to test that the code in 
> > > > > > > > > > > > > BackendUtil works to enable this pass.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This could possibly be staged as independent commits 
> > > > > > > > > > > > > - adding the LLVM functionality in one commit, which 
> > > > > > > > > > > > > would be a no-op for Clang because it wouldn't be 
> > > > > > > > > > > > > setting PTO.UniqueLinkageNames - then committing the 
> > > > > > > > > > > > > Clang change that would remove the custom pass 
> > > > > > > > > > > > > addition and set PTO.UniqueLinkageNames - and then 
> > > > > > > > > > > > > it'd probably be reasonable to have this test be made 
> > > > > > > > > > > > > a bit more explicit (testing the pass manager 
> > > > > > > > > > > > > structure/order) to show that that Clang change had 
> > > > > > > > > > > > > an effect: Moving the pass to the desired location in 
> > > > > > > > > > > > > the pass pipeline.
> > > > > > > > > > > > This is a good question. No, this test does not 
> > > > > > > > > > > > validate the pipeline change on the LLVM side, since 
> > > > > > > > > > > > Clang shouldn't have knowledge about how the pipelines 
> > > > > > > > > > > > are arranged in LLVM. As you pointed out, the test here 
> > > > > > > > > > > > is to test if the specific pass is run and gives 
> > > > > > > > > > > > expected results.
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks for the suggestion to break the Clang changes 
> > > > > > > > > > > > and LLVM changes apart which would make the testing 
> > > > > > > > > > > > more specific. The pipeline ordering could be tested 
> > > > > > > > > > > > with a LLVM test but that would require a LLVM switch 
> > > > > > > > > > > > setup for UniqueLinkageNames and I'm not sure there's a 
> > > > > > > > > > > > need for that switch except for testing.
> > > > > > > > > > > > No, this test does not validate the pipeline change on 
> > > > > > > > > > > > the LLVM side, since Clang shouldn't have knowledge 
> > > > > > > > > > > > about how the pipelines are arranged in LLVM. 
> > > > > > > > > > > 
> > > > > > > > > > > "ish" - but Clang should have tests for changes to Clang, 
> > > > > > > > > > > ideally. Usually they can simply be testing LLVM's IR 
> > > > > > > > > > > output before it goes to LLVM for optimization/codegen - 
> > > > > > > > > > > but for features that don't have this serialization 
> > > > > > > > > > > boundary that makes testing and isolation clear/simple, 
> > > > > > > > > > > it becomes a bit fuzzier.
> > > > > > > > > > > 
> > > > > > > > > > > In this case, there is a clang change - from adding the 
> > > > > > > > > > > pass explicitly in Clang, to setting a parameter about 
> > > > > > > > > > > how LLVM will add the pass, and it has an observable 
> > > > > > > > > > > effect. One way to test this change while isolating the 
> > > > > > > > > > > Clang test from further changes to the pipeline in LLVM, 
> > > > > > > > > > > would be to test that the pass ends up somewhere in the 
> > > > > > > > > > > LLVM-created part of the pass pipeline - the parts that 
> > > > > > > > > > > you can't get to from the way the original pass addition 
> > > > > > > > > > > was written in Clang. At least I assume that's the 
> > > > > > > > > > > case/what motivated the change from adding it in Clang to 
> > > > > > > > > > > adding it in LLVM?
> > > > > > > > > > > 
> > > > > > > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is 
> > > > > > > > > > > able to add passes before/after, say it always adds 'a' 
> > > > > > > > > > > before and 'b' after, to make {a, x, y, z, b} - and this 
> > > > > > > > > > > new pass u was previously added at the start to make {u, 
> > > > > > > > > > > a, x, y, z, b} but now needs to go in {a, x, y, u, z, b} 
> > > > > > > > > > > you could test that 'u' is after 'a' and before 'b', or 
> > > > > > > > > > > between 'x' and 'z', etc. If there's some other more 
> > > > > > > > > > > clear/simple

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

dblaikie wrote:
> hoy wrote:
> > dblaikie wrote:
> > > aeubanks wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > aeubanks wrote:
> > > > > > > hoy wrote:
> > > > > > > > aeubanks wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > hoy wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > Does this test validate the new behavior? (ie: does 
> > > > > > > > > > > > this test fail without the LLVM changes and pass with 
> > > > > > > > > > > > it) Not that it necessarily has to - since Clang isn't 
> > > > > > > > > > > > here to test the LLVM behavior - perhaps this test is 
> > > > > > > > > > > > sufficient in Clang to test that the code in 
> > > > > > > > > > > > BackendUtil works to enable this pass.
> > > > > > > > > > > > 
> > > > > > > > > > > > This could possibly be staged as independent commits - 
> > > > > > > > > > > > adding the LLVM functionality in one commit, which 
> > > > > > > > > > > > would be a no-op for Clang because it wouldn't be 
> > > > > > > > > > > > setting PTO.UniqueLinkageNames - then committing the 
> > > > > > > > > > > > Clang change that would remove the custom pass addition 
> > > > > > > > > > > > and set PTO.UniqueLinkageNames - and then it'd probably 
> > > > > > > > > > > > be reasonable to have this test be made a bit more 
> > > > > > > > > > > > explicit (testing the pass manager structure/order) to 
> > > > > > > > > > > > show that that Clang change had an effect: Moving the 
> > > > > > > > > > > > pass to the desired location in the pass pipeline.
> > > > > > > > > > > This is a good question. No, this test does not validate 
> > > > > > > > > > > the pipeline change on the LLVM side, since Clang 
> > > > > > > > > > > shouldn't have knowledge about how the pipelines are 
> > > > > > > > > > > arranged in LLVM. As you pointed out, the test here is to 
> > > > > > > > > > > test if the specific pass is run and gives expected 
> > > > > > > > > > > results.
> > > > > > > > > > > 
> > > > > > > > > > > Thanks for the suggestion to break the Clang changes and 
> > > > > > > > > > > LLVM changes apart which would make the testing more 
> > > > > > > > > > > specific. The pipeline ordering could be tested with a 
> > > > > > > > > > > LLVM test but that would require a LLVM switch setup for 
> > > > > > > > > > > UniqueLinkageNames and I'm not sure there's a need for 
> > > > > > > > > > > that switch except for testing.
> > > > > > > > > > > No, this test does not validate the pipeline change on 
> > > > > > > > > > > the LLVM side, since Clang shouldn't have knowledge about 
> > > > > > > > > > > how the pipelines are arranged in LLVM. 
> > > > > > > > > > 
> > > > > > > > > > "ish" - but Clang should have tests for changes to Clang, 
> > > > > > > > > > ideally. Usually they can simply be testing LLVM's IR 
> > > > > > > > > > output before it goes to LLVM for optimization/codegen - 
> > > > > > > > > > but for features that don't have this serialization 
> > > > > > > > > > boundary that makes testing and isolation clear/simple, it 
> > > > > > > > > > becomes a bit fuzzier.
> > > > > > > > > > 
> > > > > > > > > > In this case, there is a clang change - from adding the 
> > > > > > > > > > pass explicitly in Clang, to setting a parameter about how 
> > > > > > > > > > LLVM will add the pass, and it has an observable effect. 
> > > > > > > > > > One way to test this change while isolating the Clang test 
> > > > > > > > > > from further changes to the pipeline in LLVM, would be to 
> > > > > > > > > > test that the pass ends up somewhere in the LLVM-created 
> > > > > > > > > > part of the pass pipeline - the parts that you can't get to 
> > > > > > > > > > from the way the original pass addition was written in 
> > > > > > > > > > Clang. At least I assume that's the case/what motivated the 
> > > > > > > > > > change from adding it in Clang to adding it in LLVM?
> > > > > > > > > > 
> > > > > > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able 
> > > > > > > > > > to add passes before/after, say it always adds 'a' before 
> > > > > > > > > > and 'b' after, to make {a, x, y, z, b} - and this new pass 
> > > > > > > > > > u was previously added at the start to make {u, a, x, y, z, 
> > > > > > > > > > b} but now needs to go in {a, x, y, u, z, b} you could test 
> > > > > > > > > > that 'u' is after 'a' and before 'b', or between 'x' and 
> > > > > > > > > > 'z', etc. If there's some other more clear/simple/reliable 
> > > > > > > > > > marker of where the LLVM-created passes start/end in the 
> > > > > > > > > > structured dump, that'd be good to use as a landmark to 
> > > > > > > > > > make such a test more robust. If there's some

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

hoy wrote:
> dblaikie wrote:
> > aeubanks wrote:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > aeubanks wrote:
> > > > > > hoy wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > hoy wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > Does this test validate the new behavior? (ie: does this 
> > > > > > > > > > > test fail without the LLVM changes and pass with it) Not 
> > > > > > > > > > > that it necessarily has to - since Clang isn't here to 
> > > > > > > > > > > test the LLVM behavior - perhaps this test is sufficient 
> > > > > > > > > > > in Clang to test that the code in BackendUtil works to 
> > > > > > > > > > > enable this pass.
> > > > > > > > > > > 
> > > > > > > > > > > This could possibly be staged as independent commits - 
> > > > > > > > > > > adding the LLVM functionality in one commit, which would 
> > > > > > > > > > > be a no-op for Clang because it wouldn't be setting 
> > > > > > > > > > > PTO.UniqueLinkageNames - then committing the Clang change 
> > > > > > > > > > > that would remove the custom pass addition and set 
> > > > > > > > > > > PTO.UniqueLinkageNames - and then it'd probably be 
> > > > > > > > > > > reasonable to have this test be made a bit more explicit 
> > > > > > > > > > > (testing the pass manager structure/order) to show that 
> > > > > > > > > > > that Clang change had an effect: Moving the pass to the 
> > > > > > > > > > > desired location in the pass pipeline.
> > > > > > > > > > This is a good question. No, this test does not validate 
> > > > > > > > > > the pipeline change on the LLVM side, since Clang shouldn't 
> > > > > > > > > > have knowledge about how the pipelines are arranged in 
> > > > > > > > > > LLVM. As you pointed out, the test here is to test if the 
> > > > > > > > > > specific pass is run and gives expected results.
> > > > > > > > > > 
> > > > > > > > > > Thanks for the suggestion to break the Clang changes and 
> > > > > > > > > > LLVM changes apart which would make the testing more 
> > > > > > > > > > specific. The pipeline ordering could be tested with a LLVM 
> > > > > > > > > > test but that would require a LLVM switch setup for 
> > > > > > > > > > UniqueLinkageNames and I'm not sure there's a need for that 
> > > > > > > > > > switch except for testing.
> > > > > > > > > > No, this test does not validate the pipeline change on the 
> > > > > > > > > > LLVM side, since Clang shouldn't have knowledge about how 
> > > > > > > > > > the pipelines are arranged in LLVM. 
> > > > > > > > > 
> > > > > > > > > "ish" - but Clang should have tests for changes to Clang, 
> > > > > > > > > ideally. Usually they can simply be testing LLVM's IR output 
> > > > > > > > > before it goes to LLVM for optimization/codegen - but for 
> > > > > > > > > features that don't have this serialization boundary that 
> > > > > > > > > makes testing and isolation clear/simple, it becomes a bit 
> > > > > > > > > fuzzier.
> > > > > > > > > 
> > > > > > > > > In this case, there is a clang change - from adding the pass 
> > > > > > > > > explicitly in Clang, to setting a parameter about how LLVM 
> > > > > > > > > will add the pass, and it has an observable effect. One way 
> > > > > > > > > to test this change while isolating the Clang test from 
> > > > > > > > > further changes to the pipeline in LLVM, would be to test 
> > > > > > > > > that the pass ends up somewhere in the LLVM-created part of 
> > > > > > > > > the pass pipeline - the parts that you can't get to from the 
> > > > > > > > > way the original pass addition was written in Clang. At least 
> > > > > > > > > I assume that's the case/what motivated the change from 
> > > > > > > > > adding it in Clang to adding it in LLVM?
> > > > > > > > > 
> > > > > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able 
> > > > > > > > > to add passes before/after, say it always adds 'a' before and 
> > > > > > > > > 'b' after, to make {a, x, y, z, b} - and this new pass u was 
> > > > > > > > > previously added at the start to make {u, a, x, y, z, b} but 
> > > > > > > > > now needs to go in {a, x, y, u, z, b} you could test that 'u' 
> > > > > > > > > is after 'a' and before 'b', or between 'x' and 'z', etc. If 
> > > > > > > > > there's some other more clear/simple/reliable marker of where 
> > > > > > > > > the LLVM-created passes start/end in the structured dump, 
> > > > > > > > > that'd be good to use as a landmark to make such a test more 
> > > > > > > > > robust. If there's some meaningful pass that this pass always 
> > > > > > > > > needs to go after - testing that might be OK, even if it's 
> > > > > > > > > somewhat an implementation detail of LLVM 

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

[Arthur Eubanks via Phabricator via cfe-commits]

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

LGTM with nit, but somebody else more familiar with this should LGTM to make 
sure this makes sense




Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:18
+; O0: Running pass: UniqueInternalLinkageNamesPass
+; O0: Invalidating analysis: VerifierAnalysis
+

VerifierAnalysis also probably isn't necessary to check, same below


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313650.
hoy marked an inline comment as done.
hoy added a comment.

Removing unnecessary test code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -133,6 +133,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -246,6 +253,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -281,6 +291,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,25 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+; O0: Invalidating analysis: VerifierAnalysis
+
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Invalidating analysis: VerifierAnalysis
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -284,6 +284,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
+  UniqueLinkageNames = false;
 }
 
 extern cl::opt EnableConstraintElimination;
@@ -1001,6 +1002,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1770,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/P

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

[Hongtao Yu via Phabricator via cfe-commits]

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



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:20
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")

aeubanks wrote:
> is the debug info necessary for the test? it passes without it.
It's not needed. Will remove.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:39
+
+; O0: Running pass: VerifierPass
+; O0: Running analysis: VerifierAnalysis

aeubanks wrote:
> I don't think we need to check for Verifiers
It shows up as the first pass of the O0 pipeline and I use it as a reference. 
Will remove.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:44
+
+; O2: Running pass: ForceFunctionAttrsPass
+; O2: Running pass: UniqueInternalLinkageNamesPass

aeubanks wrote:
> why check ForceFunctionAttrsPass?
Like VerifierPass above, I was using it as a reference.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:47
+; O2: Invalidating analysis: VerifierAnalysis
+; O2: Running pass: SampleProfileProbePass
+

aeubanks wrote:
> should this be run for O0?
There is no need for that as this point and running it under O0 is not tested.


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.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:20
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")

is the debug info necessary for the test? it passes without it.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:39
+
+; O0: Running pass: VerifierPass
+; O0: Running analysis: VerifierAnalysis

I don't think we need to check for Verifiers



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:44
+
+; O2: Running pass: ForceFunctionAttrsPass
+; O2: Running pass: UniqueInternalLinkageNamesPass

why check ForceFunctionAttrsPass?



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:47
+; O2: Invalidating analysis: VerifierAnalysis
+; O2: Running pass: SampleProfileProbePass
+

should this be run for O0?


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313630.
hoy added a comment.

Adding PTO checks in LLVM test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -133,6 +133,13 @@
 static cl::opt DebugInfoForProfiling(
 "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
+static cl::opt PseudoProbeForProfiling(
+"new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+cl::desc("Emit pseudo probes to enable PGO profile generation."));
+static cl::opt UniqueInternalLinkageNames(
+"new-pm-unique-internal-linkage-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify Internal Linkage Symbol Names by appending the MD5 "
+ "hash of the module path."));
 /// @}}
 
 template 
@@ -246,6 +253,9 @@
 if (DebugInfoForProfiling)
   P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
  true);
+else if (PseudoProbeForProfiling)
+  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
 else
   P = None;
   }
@@ -281,6 +291,7 @@
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
+  PTO.UniqueLinkageNames = UniqueInternalLinkageNames;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,50 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0, !dbg !18
+}
+
+define dso_local i32 (...)* @bar() !dbg !7 {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*), !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, null}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 10, column: 3, scope: !7)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DILocation(line: 6, column: 3, scope: !15)
+
+; O0: Running pass: VerifierPass
+; O0: Running analysis: VerifierAnalysis
+; O0: Running pass: UniqueInternalLinkageNamesPass
+; O0: Invalidating analysis: VerifierAnalysis
+
+; O2: Running pass: ForceFunctionAttrsPass
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Invalidating analysis: VerifierAnalysis
+; O2: Running pass: SampleProfileProbePass
+
+; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cp

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

dblaikie wrote:
> aeubanks wrote:
> > dblaikie wrote:
> > > hoy wrote:
> > > > aeubanks wrote:
> > > > > hoy wrote:
> > > > > > aeubanks wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > Does this test validate the new behavior? (ie: does this 
> > > > > > > > > > test fail without the LLVM changes and pass with it) Not 
> > > > > > > > > > that it necessarily has to - since Clang isn't here to test 
> > > > > > > > > > the LLVM behavior - perhaps this test is sufficient in 
> > > > > > > > > > Clang to test that the code in BackendUtil works to enable 
> > > > > > > > > > this pass.
> > > > > > > > > > 
> > > > > > > > > > This could possibly be staged as independent commits - 
> > > > > > > > > > adding the LLVM functionality in one commit, which would be 
> > > > > > > > > > a no-op for Clang because it wouldn't be setting 
> > > > > > > > > > PTO.UniqueLinkageNames - then committing the Clang change 
> > > > > > > > > > that would remove the custom pass addition and set 
> > > > > > > > > > PTO.UniqueLinkageNames - and then it'd probably be 
> > > > > > > > > > reasonable to have this test be made a bit more explicit 
> > > > > > > > > > (testing the pass manager structure/order) to show that 
> > > > > > > > > > that Clang change had an effect: Moving the pass to the 
> > > > > > > > > > desired location in the pass pipeline.
> > > > > > > > > This is a good question. No, this test does not validate the 
> > > > > > > > > pipeline change on the LLVM side, since Clang shouldn't have 
> > > > > > > > > knowledge about how the pipelines are arranged in LLVM. As 
> > > > > > > > > you pointed out, the test here is to test if the specific 
> > > > > > > > > pass is run and gives expected results.
> > > > > > > > > 
> > > > > > > > > Thanks for the suggestion to break the Clang changes and LLVM 
> > > > > > > > > changes apart which would make the testing more specific. The 
> > > > > > > > > pipeline ordering could be tested with a LLVM test but that 
> > > > > > > > > would require a LLVM switch setup for UniqueLinkageNames and 
> > > > > > > > > I'm not sure there's a need for that switch except for 
> > > > > > > > > testing.
> > > > > > > > > No, this test does not validate the pipeline change on the 
> > > > > > > > > LLVM side, since Clang shouldn't have knowledge about how the 
> > > > > > > > > pipelines are arranged in LLVM. 
> > > > > > > > 
> > > > > > > > "ish" - but Clang should have tests for changes to Clang, 
> > > > > > > > ideally. Usually they can simply be testing LLVM's IR output 
> > > > > > > > before it goes to LLVM for optimization/codegen - but for 
> > > > > > > > features that don't have this serialization boundary that makes 
> > > > > > > > testing and isolation clear/simple, it becomes a bit fuzzier.
> > > > > > > > 
> > > > > > > > In this case, there is a clang change - from adding the pass 
> > > > > > > > explicitly in Clang, to setting a parameter about how LLVM will 
> > > > > > > > add the pass, and it has an observable effect. One way to test 
> > > > > > > > this change while isolating the Clang test from further changes 
> > > > > > > > to the pipeline in LLVM, would be to test that the pass ends up 
> > > > > > > > somewhere in the LLVM-created part of the pass pipeline - the 
> > > > > > > > parts that you can't get to from the way the original pass 
> > > > > > > > addition was written in Clang. At least I assume that's the 
> > > > > > > > case/what motivated the change from adding it in Clang to 
> > > > > > > > adding it in LLVM?
> > > > > > > > 
> > > > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able to 
> > > > > > > > add passes before/after, say it always adds 'a' before and 'b' 
> > > > > > > > after, to make {a, x, y, z, b} - and this new pass u was 
> > > > > > > > previously added at the start to make {u, a, x, y, z, b} but 
> > > > > > > > now needs to go in {a, x, y, u, z, b} you could test that 'u' 
> > > > > > > > is after 'a' and before 'b', or between 'x' and 'z', etc. If 
> > > > > > > > there's some other more clear/simple/reliable marker of where 
> > > > > > > > the LLVM-created passes start/end in the structured dump, 
> > > > > > > > that'd be good to use as a landmark to make such a test more 
> > > > > > > > robust. If there's some meaningful pass that this pass always 
> > > > > > > > needs to go after - testing that might be OK, even if it's 
> > > > > > > > somewhat an implementation detail of LLVM - whatever's likely 
> > > > > > > > to make the test more legible and more reliable/resilient to 
> > > > > > > > unrelated changes would be good.
> > > > > > > > 
> > > > > > > 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

aeubanks wrote:
> dblaikie wrote:
> > hoy wrote:
> > > aeubanks wrote:
> > > > hoy wrote:
> > > > > aeubanks wrote:
> > > > > > dblaikie wrote:
> > > > > > > hoy wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > Does this test validate the new behavior? (ie: does this test 
> > > > > > > > > fail without the LLVM changes and pass with it) Not that it 
> > > > > > > > > necessarily has to - since Clang isn't here to test the LLVM 
> > > > > > > > > behavior - perhaps this test is sufficient in Clang to test 
> > > > > > > > > that the code in BackendUtil works to enable this pass.
> > > > > > > > > 
> > > > > > > > > This could possibly be staged as independent commits - adding 
> > > > > > > > > the LLVM functionality in one commit, which would be a no-op 
> > > > > > > > > for Clang because it wouldn't be setting 
> > > > > > > > > PTO.UniqueLinkageNames - then committing the Clang change 
> > > > > > > > > that would remove the custom pass addition and set 
> > > > > > > > > PTO.UniqueLinkageNames - and then it'd probably be reasonable 
> > > > > > > > > to have this test be made a bit more explicit (testing the 
> > > > > > > > > pass manager structure/order) to show that that Clang change 
> > > > > > > > > had an effect: Moving the pass to the desired location in the 
> > > > > > > > > pass pipeline.
> > > > > > > > This is a good question. No, this test does not validate the 
> > > > > > > > pipeline change on the LLVM side, since Clang shouldn't have 
> > > > > > > > knowledge about how the pipelines are arranged in LLVM. As you 
> > > > > > > > pointed out, the test here is to test if the specific pass is 
> > > > > > > > run and gives expected results.
> > > > > > > > 
> > > > > > > > Thanks for the suggestion to break the Clang changes and LLVM 
> > > > > > > > changes apart which would make the testing more specific. The 
> > > > > > > > pipeline ordering could be tested with a LLVM test but that 
> > > > > > > > would require a LLVM switch setup for UniqueLinkageNames and 
> > > > > > > > I'm not sure there's a need for that switch except for testing.
> > > > > > > > No, this test does not validate the pipeline change on the LLVM 
> > > > > > > > side, since Clang shouldn't have knowledge about how the 
> > > > > > > > pipelines are arranged in LLVM. 
> > > > > > > 
> > > > > > > "ish" - but Clang should have tests for changes to Clang, 
> > > > > > > ideally. Usually they can simply be testing LLVM's IR output 
> > > > > > > before it goes to LLVM for optimization/codegen - but for 
> > > > > > > features that don't have this serialization boundary that makes 
> > > > > > > testing and isolation clear/simple, it becomes a bit fuzzier.
> > > > > > > 
> > > > > > > In this case, there is a clang change - from adding the pass 
> > > > > > > explicitly in Clang, to setting a parameter about how LLVM will 
> > > > > > > add the pass, and it has an observable effect. One way to test 
> > > > > > > this change while isolating the Clang test from further changes 
> > > > > > > to the pipeline in LLVM, would be to test that the pass ends up 
> > > > > > > somewhere in the LLVM-created part of the pass pipeline - the 
> > > > > > > parts that you can't get to from the way the original pass 
> > > > > > > addition was written in Clang. At least I assume that's the 
> > > > > > > case/what motivated the change from adding it in Clang to adding 
> > > > > > > it in LLVM?
> > > > > > > 
> > > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able to 
> > > > > > > add passes before/after, say it always adds 'a' before and 'b' 
> > > > > > > after, to make {a, x, y, z, b} - and this new pass u was 
> > > > > > > previously added at the start to make {u, a, x, y, z, b} but now 
> > > > > > > needs to go in {a, x, y, u, z, b} you could test that 'u' is 
> > > > > > > after 'a' and before 'b', or between 'x' and 'z', etc. If there's 
> > > > > > > some other more clear/simple/reliable marker of where the 
> > > > > > > LLVM-created passes start/end in the structured dump, that'd be 
> > > > > > > good to use as a landmark to make such a test more robust. If 
> > > > > > > there's some meaningful pass that this pass always needs to go 
> > > > > > > after - testing that might be OK, even if it's somewhat an 
> > > > > > > implementation detail of LLVM - whatever's likely to make the 
> > > > > > > test more legible and more reliable/resilient to unrelated 
> > > > > > > changes would be good.
> > > > > > > 
> > > > > > > > As you pointed out, the test here is to test if the specific 
> > > > > > > > pass is run and gives expected results.
> > > > > > > 
> > > > > > > If that's the case, this test could be committ

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

dblaikie wrote:
> hoy wrote:
> > aeubanks wrote:
> > > hoy wrote:
> > > > aeubanks wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > Does this test validate the new behavior? (ie: does this test 
> > > > > > > > fail without the LLVM changes and pass with it) Not that it 
> > > > > > > > necessarily has to - since Clang isn't here to test the LLVM 
> > > > > > > > behavior - perhaps this test is sufficient in Clang to test 
> > > > > > > > that the code in BackendUtil works to enable this pass.
> > > > > > > > 
> > > > > > > > This could possibly be staged as independent commits - adding 
> > > > > > > > the LLVM functionality in one commit, which would be a no-op 
> > > > > > > > for Clang because it wouldn't be setting PTO.UniqueLinkageNames 
> > > > > > > > - then committing the Clang change that would remove the custom 
> > > > > > > > pass addition and set PTO.UniqueLinkageNames - and then it'd 
> > > > > > > > probably be reasonable to have this test be made a bit more 
> > > > > > > > explicit (testing the pass manager structure/order) to show 
> > > > > > > > that that Clang change had an effect: Moving the pass to the 
> > > > > > > > desired location in the pass pipeline.
> > > > > > > This is a good question. No, this test does not validate the 
> > > > > > > pipeline change on the LLVM side, since Clang shouldn't have 
> > > > > > > knowledge about how the pipelines are arranged in LLVM. As you 
> > > > > > > pointed out, the test here is to test if the specific pass is run 
> > > > > > > and gives expected results.
> > > > > > > 
> > > > > > > Thanks for the suggestion to break the Clang changes and LLVM 
> > > > > > > changes apart which would make the testing more specific. The 
> > > > > > > pipeline ordering could be tested with a LLVM test but that would 
> > > > > > > require a LLVM switch setup for UniqueLinkageNames and I'm not 
> > > > > > > sure there's a need for that switch except for testing.
> > > > > > > No, this test does not validate the pipeline change on the LLVM 
> > > > > > > side, since Clang shouldn't have knowledge about how the 
> > > > > > > pipelines are arranged in LLVM. 
> > > > > > 
> > > > > > "ish" - but Clang should have tests for changes to Clang, ideally. 
> > > > > > Usually they can simply be testing LLVM's IR output before it goes 
> > > > > > to LLVM for optimization/codegen - but for features that don't have 
> > > > > > this serialization boundary that makes testing and isolation 
> > > > > > clear/simple, it becomes a bit fuzzier.
> > > > > > 
> > > > > > In this case, there is a clang change - from adding the pass 
> > > > > > explicitly in Clang, to setting a parameter about how LLVM will add 
> > > > > > the pass, and it has an observable effect. One way to test this 
> > > > > > change while isolating the Clang test from further changes to the 
> > > > > > pipeline in LLVM, would be to test that the pass ends up somewhere 
> > > > > > in the LLVM-created part of the pass pipeline - the parts that you 
> > > > > > can't get to from the way the original pass addition was written in 
> > > > > > Clang. At least I assume that's the case/what motivated the change 
> > > > > > from adding it in Clang to adding it in LLVM?
> > > > > > 
> > > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able to add 
> > > > > > passes before/after, say it always adds 'a' before and 'b' after, 
> > > > > > to make {a, x, y, z, b} - and this new pass u was previously added 
> > > > > > at the start to make {u, a, x, y, z, b} but now needs to go in {a, 
> > > > > > x, y, u, z, b} you could test that 'u' is after 'a' and before 'b', 
> > > > > > or between 'x' and 'z', etc. If there's some other more 
> > > > > > clear/simple/reliable marker of where the LLVM-created passes 
> > > > > > start/end in the structured dump, that'd be good to use as a 
> > > > > > landmark to make such a test more robust. If there's some 
> > > > > > meaningful pass that this pass always needs to go after - testing 
> > > > > > that might be OK, even if it's somewhat an implementation detail of 
> > > > > > LLVM - whatever's likely to make the test more legible and more 
> > > > > > reliable/resilient to unrelated changes would be good.
> > > > > > 
> > > > > > > As you pointed out, the test here is to test if the specific pass 
> > > > > > > is run and gives expected results.
> > > > > > 
> > > > > > If that's the case, this test could be committed standalone, before 
> > > > > > any of these other changes?
> > > > > > 
> > > > > > > The pipeline ordering could be tested with a LLVM test but that 
> > > > > > > would require a LLVM switch setup for Un

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

hoy wrote:
> aeubanks wrote:
> > hoy wrote:
> > > aeubanks wrote:
> > > > dblaikie wrote:
> > > > > hoy wrote:
> > > > > > dblaikie wrote:
> > > > > > > Does this test validate the new behavior? (ie: does this test 
> > > > > > > fail without the LLVM changes and pass with it) Not that it 
> > > > > > > necessarily has to - since Clang isn't here to test the LLVM 
> > > > > > > behavior - perhaps this test is sufficient in Clang to test that 
> > > > > > > the code in BackendUtil works to enable this pass.
> > > > > > > 
> > > > > > > This could possibly be staged as independent commits - adding the 
> > > > > > > LLVM functionality in one commit, which would be a no-op for 
> > > > > > > Clang because it wouldn't be setting PTO.UniqueLinkageNames - 
> > > > > > > then committing the Clang change that would remove the custom 
> > > > > > > pass addition and set PTO.UniqueLinkageNames - and then it'd 
> > > > > > > probably be reasonable to have this test be made a bit more 
> > > > > > > explicit (testing the pass manager structure/order) to show that 
> > > > > > > that Clang change had an effect: Moving the pass to the desired 
> > > > > > > location in the pass pipeline.
> > > > > > This is a good question. No, this test does not validate the 
> > > > > > pipeline change on the LLVM side, since Clang shouldn't have 
> > > > > > knowledge about how the pipelines are arranged in LLVM. As you 
> > > > > > pointed out, the test here is to test if the specific pass is run 
> > > > > > and gives expected results.
> > > > > > 
> > > > > > Thanks for the suggestion to break the Clang changes and LLVM 
> > > > > > changes apart which would make the testing more specific. The 
> > > > > > pipeline ordering could be tested with a LLVM test but that would 
> > > > > > require a LLVM switch setup for UniqueLinkageNames and I'm not sure 
> > > > > > there's a need for that switch except for testing.
> > > > > > No, this test does not validate the pipeline change on the LLVM 
> > > > > > side, since Clang shouldn't have knowledge about how the pipelines 
> > > > > > are arranged in LLVM. 
> > > > > 
> > > > > "ish" - but Clang should have tests for changes to Clang, ideally. 
> > > > > Usually they can simply be testing LLVM's IR output before it goes to 
> > > > > LLVM for optimization/codegen - but for features that don't have this 
> > > > > serialization boundary that makes testing and isolation clear/simple, 
> > > > > it becomes a bit fuzzier.
> > > > > 
> > > > > In this case, there is a clang change - from adding the pass 
> > > > > explicitly in Clang, to setting a parameter about how LLVM will add 
> > > > > the pass, and it has an observable effect. One way to test this 
> > > > > change while isolating the Clang test from further changes to the 
> > > > > pipeline in LLVM, would be to test that the pass ends up somewhere in 
> > > > > the LLVM-created part of the pass pipeline - the parts that you can't 
> > > > > get to from the way the original pass addition was written in Clang. 
> > > > > At least I assume that's the case/what motivated the change from 
> > > > > adding it in Clang to adding it in LLVM?
> > > > > 
> > > > > eg: if LLVM always forms passes {x, y, z} and Clang is able to add 
> > > > > passes before/after, say it always adds 'a' before and 'b' after, to 
> > > > > make {a, x, y, z, b} - and this new pass u was previously added at 
> > > > > the start to make {u, a, x, y, z, b} but now needs to go in {a, x, y, 
> > > > > u, z, b} you could test that 'u' is after 'a' and before 'b', or 
> > > > > between 'x' and 'z', etc. If there's some other more 
> > > > > clear/simple/reliable marker of where the LLVM-created passes 
> > > > > start/end in the structured dump, that'd be good to use as a landmark 
> > > > > to make such a test more robust. If there's some meaningful pass that 
> > > > > this pass always needs to go after - testing that might be OK, even 
> > > > > if it's somewhat an implementation detail of LLVM - whatever's likely 
> > > > > to make the test more legible and more reliable/resilient to 
> > > > > unrelated changes would be good.
> > > > > 
> > > > > > As you pointed out, the test here is to test if the specific pass 
> > > > > > is run and gives expected results.
> > > > > 
> > > > > If that's the case, this test could be committed standalone, before 
> > > > > any of these other changes?
> > > > > 
> > > > > > The pipeline ordering could be tested with a LLVM test but that 
> > > > > > would require a LLVM switch setup for UniqueLinkageNames and I'm 
> > > > > > not sure there's a need for that switch except for testing.
> > > > > 
> > > > > That's OK, the entire 'opt' tool and all its sw

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

aeubanks wrote:
> hoy wrote:
> > aeubanks wrote:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > dblaikie wrote:
> > > > > > Does this test validate the new behavior? (ie: does this test fail 
> > > > > > without the LLVM changes and pass with it) Not that it necessarily 
> > > > > > has to - since Clang isn't here to test the LLVM behavior - perhaps 
> > > > > > this test is sufficient in Clang to test that the code in 
> > > > > > BackendUtil works to enable this pass.
> > > > > > 
> > > > > > This could possibly be staged as independent commits - adding the 
> > > > > > LLVM functionality in one commit, which would be a no-op for Clang 
> > > > > > because it wouldn't be setting PTO.UniqueLinkageNames - then 
> > > > > > committing the Clang change that would remove the custom pass 
> > > > > > addition and set PTO.UniqueLinkageNames - and then it'd probably be 
> > > > > > reasonable to have this test be made a bit more explicit (testing 
> > > > > > the pass manager structure/order) to show that that Clang change 
> > > > > > had an effect: Moving the pass to the desired location in the pass 
> > > > > > pipeline.
> > > > > This is a good question. No, this test does not validate the pipeline 
> > > > > change on the LLVM side, since Clang shouldn't have knowledge about 
> > > > > how the pipelines are arranged in LLVM. As you pointed out, the test 
> > > > > here is to test if the specific pass is run and gives expected 
> > > > > results.
> > > > > 
> > > > > Thanks for the suggestion to break the Clang changes and LLVM changes 
> > > > > apart which would make the testing more specific. The pipeline 
> > > > > ordering could be tested with a LLVM test but that would require a 
> > > > > LLVM switch setup for UniqueLinkageNames and I'm not sure there's a 
> > > > > need for that switch except for testing.
> > > > > No, this test does not validate the pipeline change on the LLVM side, 
> > > > > since Clang shouldn't have knowledge about how the pipelines are 
> > > > > arranged in LLVM. 
> > > > 
> > > > "ish" - but Clang should have tests for changes to Clang, ideally. 
> > > > Usually they can simply be testing LLVM's IR output before it goes to 
> > > > LLVM for optimization/codegen - but for features that don't have this 
> > > > serialization boundary that makes testing and isolation clear/simple, 
> > > > it becomes a bit fuzzier.
> > > > 
> > > > In this case, there is a clang change - from adding the pass explicitly 
> > > > in Clang, to setting a parameter about how LLVM will add the pass, and 
> > > > it has an observable effect. One way to test this change while 
> > > > isolating the Clang test from further changes to the pipeline in LLVM, 
> > > > would be to test that the pass ends up somewhere in the LLVM-created 
> > > > part of the pass pipeline - the parts that you can't get to from the 
> > > > way the original pass addition was written in Clang. At least I assume 
> > > > that's the case/what motivated the change from adding it in Clang to 
> > > > adding it in LLVM?
> > > > 
> > > > eg: if LLVM always forms passes {x, y, z} and Clang is able to add 
> > > > passes before/after, say it always adds 'a' before and 'b' after, to 
> > > > make {a, x, y, z, b} - and this new pass u was previously added at the 
> > > > start to make {u, a, x, y, z, b} but now needs to go in {a, x, y, u, z, 
> > > > b} you could test that 'u' is after 'a' and before 'b', or between 'x' 
> > > > and 'z', etc. If there's some other more clear/simple/reliable marker 
> > > > of where the LLVM-created passes start/end in the structured dump, 
> > > > that'd be good to use as a landmark to make such a test more robust. If 
> > > > there's some meaningful pass that this pass always needs to go after - 
> > > > testing that might be OK, even if it's somewhat an implementation 
> > > > detail of LLVM - whatever's likely to make the test more legible and 
> > > > more reliable/resilient to unrelated changes would be good.
> > > > 
> > > > > As you pointed out, the test here is to test if the specific pass is 
> > > > > run and gives expected results.
> > > > 
> > > > If that's the case, this test could be committed standalone, before any 
> > > > of these other changes?
> > > > 
> > > > > The pipeline ordering could be tested with a LLVM test but that would 
> > > > > require a LLVM switch setup for UniqueLinkageNames and I'm not sure 
> > > > > there's a need for that switch except for testing.
> > > > 
> > > > That's OK, the entire 'opt' tool and all its switches only exist for 
> > > > testing. eg: 
> > > > https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284
> > > The point of this change is that Un

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

hoy wrote:
> aeubanks wrote:
> > dblaikie wrote:
> > > hoy wrote:
> > > > dblaikie wrote:
> > > > > Does this test validate the new behavior? (ie: does this test fail 
> > > > > without the LLVM changes and pass with it) Not that it necessarily 
> > > > > has to - since Clang isn't here to test the LLVM behavior - perhaps 
> > > > > this test is sufficient in Clang to test that the code in BackendUtil 
> > > > > works to enable this pass.
> > > > > 
> > > > > This could possibly be staged as independent commits - adding the 
> > > > > LLVM functionality in one commit, which would be a no-op for Clang 
> > > > > because it wouldn't be setting PTO.UniqueLinkageNames - then 
> > > > > committing the Clang change that would remove the custom pass 
> > > > > addition and set PTO.UniqueLinkageNames - and then it'd probably be 
> > > > > reasonable to have this test be made a bit more explicit (testing the 
> > > > > pass manager structure/order) to show that that Clang change had an 
> > > > > effect: Moving the pass to the desired location in the pass pipeline.
> > > > This is a good question. No, this test does not validate the pipeline 
> > > > change on the LLVM side, since Clang shouldn't have knowledge about how 
> > > > the pipelines are arranged in LLVM. As you pointed out, the test here 
> > > > is to test if the specific pass is run and gives expected results.
> > > > 
> > > > Thanks for the suggestion to break the Clang changes and LLVM changes 
> > > > apart which would make the testing more specific. The pipeline ordering 
> > > > could be tested with a LLVM test but that would require a LLVM switch 
> > > > setup for UniqueLinkageNames and I'm not sure there's a need for that 
> > > > switch except for testing.
> > > > No, this test does not validate the pipeline change on the LLVM side, 
> > > > since Clang shouldn't have knowledge about how the pipelines are 
> > > > arranged in LLVM. 
> > > 
> > > "ish" - but Clang should have tests for changes to Clang, ideally. 
> > > Usually they can simply be testing LLVM's IR output before it goes to 
> > > LLVM for optimization/codegen - but for features that don't have this 
> > > serialization boundary that makes testing and isolation clear/simple, it 
> > > becomes a bit fuzzier.
> > > 
> > > In this case, there is a clang change - from adding the pass explicitly 
> > > in Clang, to setting a parameter about how LLVM will add the pass, and it 
> > > has an observable effect. One way to test this change while isolating the 
> > > Clang test from further changes to the pipeline in LLVM, would be to test 
> > > that the pass ends up somewhere in the LLVM-created part of the pass 
> > > pipeline - the parts that you can't get to from the way the original pass 
> > > addition was written in Clang. At least I assume that's the case/what 
> > > motivated the change from adding it in Clang to adding it in LLVM?
> > > 
> > > eg: if LLVM always forms passes {x, y, z} and Clang is able to add passes 
> > > before/after, say it always adds 'a' before and 'b' after, to make {a, x, 
> > > y, z, b} - and this new pass u was previously added at the start to make 
> > > {u, a, x, y, z, b} but now needs to go in {a, x, y, u, z, b} you could 
> > > test that 'u' is after 'a' and before 'b', or between 'x' and 'z', etc. 
> > > If there's some other more clear/simple/reliable marker of where the 
> > > LLVM-created passes start/end in the structured dump, that'd be good to 
> > > use as a landmark to make such a test more robust. If there's some 
> > > meaningful pass that this pass always needs to go after - testing that 
> > > might be OK, even if it's somewhat an implementation detail of LLVM - 
> > > whatever's likely to make the test more legible and more 
> > > reliable/resilient to unrelated changes would be good.
> > > 
> > > > As you pointed out, the test here is to test if the specific pass is 
> > > > run and gives expected results.
> > > 
> > > If that's the case, this test could be committed standalone, before any 
> > > of these other changes?
> > > 
> > > > The pipeline ordering could be tested with a LLVM test but that would 
> > > > require a LLVM switch setup for UniqueLinkageNames and I'm not sure 
> > > > there's a need for that switch except for testing.
> > > 
> > > That's OK, the entire 'opt' tool and all its switches only exist for 
> > > testing. eg: 
> > > https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284
> > The point of this change is that UniqueInternalLinkageNamesPass should run 
> > before SampleProfileProbePass. That must make a difference in the output of 
> > something like `clang -emit-llvm -O1`, right? Maybe we can add a new

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

aeubanks wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > Does this test validate the new behavior? (ie: does this test fail 
> > > > without the LLVM changes and pass with it) Not that it necessarily has 
> > > > to - since Clang isn't here to test the LLVM behavior - perhaps this 
> > > > test is sufficient in Clang to test that the code in BackendUtil works 
> > > > to enable this pass.
> > > > 
> > > > This could possibly be staged as independent commits - adding the LLVM 
> > > > functionality in one commit, which would be a no-op for Clang because 
> > > > it wouldn't be setting PTO.UniqueLinkageNames - then committing the 
> > > > Clang change that would remove the custom pass addition and set 
> > > > PTO.UniqueLinkageNames - and then it'd probably be reasonable to have 
> > > > this test be made a bit more explicit (testing the pass manager 
> > > > structure/order) to show that that Clang change had an effect: Moving 
> > > > the pass to the desired location in the pass pipeline.
> > > This is a good question. No, this test does not validate the pipeline 
> > > change on the LLVM side, since Clang shouldn't have knowledge about how 
> > > the pipelines are arranged in LLVM. As you pointed out, the test here is 
> > > to test if the specific pass is run and gives expected results.
> > > 
> > > Thanks for the suggestion to break the Clang changes and LLVM changes 
> > > apart which would make the testing more specific. The pipeline ordering 
> > > could be tested with a LLVM test but that would require a LLVM switch 
> > > setup for UniqueLinkageNames and I'm not sure there's a need for that 
> > > switch except for testing.
> > > No, this test does not validate the pipeline change on the LLVM side, 
> > > since Clang shouldn't have knowledge about how the pipelines are arranged 
> > > in LLVM. 
> > 
> > "ish" - but Clang should have tests for changes to Clang, ideally. Usually 
> > they can simply be testing LLVM's IR output before it goes to LLVM for 
> > optimization/codegen - but for features that don't have this serialization 
> > boundary that makes testing and isolation clear/simple, it becomes a bit 
> > fuzzier.
> > 
> > In this case, there is a clang change - from adding the pass explicitly in 
> > Clang, to setting a parameter about how LLVM will add the pass, and it has 
> > an observable effect. One way to test this change while isolating the Clang 
> > test from further changes to the pipeline in LLVM, would be to test that 
> > the pass ends up somewhere in the LLVM-created part of the pass pipeline - 
> > the parts that you can't get to from the way the original pass addition was 
> > written in Clang. At least I assume that's the case/what motivated the 
> > change from adding it in Clang to adding it in LLVM?
> > 
> > eg: if LLVM always forms passes {x, y, z} and Clang is able to add passes 
> > before/after, say it always adds 'a' before and 'b' after, to make {a, x, 
> > y, z, b} - and this new pass u was previously added at the start to make 
> > {u, a, x, y, z, b} but now needs to go in {a, x, y, u, z, b} you could test 
> > that 'u' is after 'a' and before 'b', or between 'x' and 'z', etc. If 
> > there's some other more clear/simple/reliable marker of where the 
> > LLVM-created passes start/end in the structured dump, that'd be good to use 
> > as a landmark to make such a test more robust. If there's some meaningful 
> > pass that this pass always needs to go after - testing that might be OK, 
> > even if it's somewhat an implementation detail of LLVM - whatever's likely 
> > to make the test more legible and more reliable/resilient to unrelated 
> > changes would be good.
> > 
> > > As you pointed out, the test here is to test if the specific pass is run 
> > > and gives expected results.
> > 
> > If that's the case, this test could be committed standalone, before any of 
> > these other changes?
> > 
> > > The pipeline ordering could be tested with a LLVM test but that would 
> > > require a LLVM switch setup for UniqueLinkageNames and I'm not sure 
> > > there's a need for that switch except for testing.
> > 
> > That's OK, the entire 'opt' tool and all its switches only exist for 
> > testing. eg: 
> > https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284
> The point of this change is that UniqueInternalLinkageNamesPass should run 
> before SampleProfileProbePass. That must make a difference in the output of 
> something like `clang -emit-llvm -O1`, right? Maybe we can add a new clang 
> test that checks for that new change in IR, no need to check 
> -fdebug-pass-manager. (I'm not familiar with the passes, correct me if I'm 
> wrong)
Maybe we 

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

dblaikie wrote:
> hoy wrote:
> > dblaikie wrote:
> > > Does this test validate the new behavior? (ie: does this test fail 
> > > without the LLVM changes and pass with it) Not that it necessarily has to 
> > > - since Clang isn't here to test the LLVM behavior - perhaps this test is 
> > > sufficient in Clang to test that the code in BackendUtil works to enable 
> > > this pass.
> > > 
> > > This could possibly be staged as independent commits - adding the LLVM 
> > > functionality in one commit, which would be a no-op for Clang because it 
> > > wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang 
> > > change that would remove the custom pass addition and set 
> > > PTO.UniqueLinkageNames - and then it'd probably be reasonable to have 
> > > this test be made a bit more explicit (testing the pass manager 
> > > structure/order) to show that that Clang change had an effect: Moving the 
> > > pass to the desired location in the pass pipeline.
> > This is a good question. No, this test does not validate the pipeline 
> > change on the LLVM side, since Clang shouldn't have knowledge about how the 
> > pipelines are arranged in LLVM. As you pointed out, the test here is to 
> > test if the specific pass is run and gives expected results.
> > 
> > Thanks for the suggestion to break the Clang changes and LLVM changes apart 
> > which would make the testing more specific. The pipeline ordering could be 
> > tested with a LLVM test but that would require a LLVM switch setup for 
> > UniqueLinkageNames and I'm not sure there's a need for that switch except 
> > for testing.
> > No, this test does not validate the pipeline change on the LLVM side, since 
> > Clang shouldn't have knowledge about how the pipelines are arranged in 
> > LLVM. 
> 
> "ish" - but Clang should have tests for changes to Clang, ideally. Usually 
> they can simply be testing LLVM's IR output before it goes to LLVM for 
> optimization/codegen - but for features that don't have this serialization 
> boundary that makes testing and isolation clear/simple, it becomes a bit 
> fuzzier.
> 
> In this case, there is a clang change - from adding the pass explicitly in 
> Clang, to setting a parameter about how LLVM will add the pass, and it has an 
> observable effect. One way to test this change while isolating the Clang test 
> from further changes to the pipeline in LLVM, would be to test that the pass 
> ends up somewhere in the LLVM-created part of the pass pipeline - the parts 
> that you can't get to from the way the original pass addition was written in 
> Clang. At least I assume that's the case/what motivated the change from 
> adding it in Clang to adding it in LLVM?
> 
> eg: if LLVM always forms passes {x, y, z} and Clang is able to add passes 
> before/after, say it always adds 'a' before and 'b' after, to make {a, x, y, 
> z, b} - and this new pass u was previously added at the start to make {u, a, 
> x, y, z, b} but now needs to go in {a, x, y, u, z, b} you could test that 'u' 
> is after 'a' and before 'b', or between 'x' and 'z', etc. If there's some 
> other more clear/simple/reliable marker of where the LLVM-created passes 
> start/end in the structured dump, that'd be good to use as a landmark to make 
> such a test more robust. If there's some meaningful pass that this pass 
> always needs to go after - testing that might be OK, even if it's somewhat an 
> implementation detail of LLVM - whatever's likely to make the test more 
> legible and more reliable/resilient to unrelated changes would be good.
> 
> > As you pointed out, the test here is to test if the specific pass is run 
> > and gives expected results.
> 
> If that's the case, this test could be committed standalone, before any of 
> these other changes?
> 
> > The pipeline ordering could be tested with a LLVM test but that would 
> > require a LLVM switch setup for UniqueLinkageNames and I'm not sure there's 
> > a need for that switch except for testing.
> 
> That's OK, the entire 'opt' tool and all its switches only exist for testing. 
> eg: 
> https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284
The point of this change is that UniqueInternalLinkageNamesPass should run 
before SampleProfileProbePass. That must make a difference in the output of 
something like `clang -emit-llvm -O1`, right? Maybe we can add a new clang test 
that checks for that new change in IR, no need to check -fdebug-pass-manager. 
(I'm not familiar with the passes, correct me if I'm wrong)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

hoy wrote:
> dblaikie wrote:
> > Does this test validate the new behavior? (ie: does this test fail without 
> > the LLVM changes and pass with it) Not that it necessarily has to - since 
> > Clang isn't here to test the LLVM behavior - perhaps this test is 
> > sufficient in Clang to test that the code in BackendUtil works to enable 
> > this pass.
> > 
> > This could possibly be staged as independent commits - adding the LLVM 
> > functionality in one commit, which would be a no-op for Clang because it 
> > wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang 
> > change that would remove the custom pass addition and set 
> > PTO.UniqueLinkageNames - and then it'd probably be reasonable to have this 
> > test be made a bit more explicit (testing the pass manager structure/order) 
> > to show that that Clang change had an effect: Moving the pass to the 
> > desired location in the pass pipeline.
> This is a good question. No, this test does not validate the pipeline change 
> on the LLVM side, since Clang shouldn't have knowledge about how the 
> pipelines are arranged in LLVM. As you pointed out, the test here is to test 
> if the specific pass is run and gives expected results.
> 
> Thanks for the suggestion to break the Clang changes and LLVM changes apart 
> which would make the testing more specific. The pipeline ordering could be 
> tested with a LLVM test but that would require a LLVM switch setup for 
> UniqueLinkageNames and I'm not sure there's a need for that switch except for 
> testing.
> No, this test does not validate the pipeline change on the LLVM side, since 
> Clang shouldn't have knowledge about how the pipelines are arranged in LLVM. 

"ish" - but Clang should have tests for changes to Clang, ideally. Usually they 
can simply be testing LLVM's IR output before it goes to LLVM for 
optimization/codegen - but for features that don't have this serialization 
boundary that makes testing and isolation clear/simple, it becomes a bit 
fuzzier.

In this case, there is a clang change - from adding the pass explicitly in 
Clang, to setting a parameter about how LLVM will add the pass, and it has an 
observable effect. One way to test this change while isolating the Clang test 
from further changes to the pipeline in LLVM, would be to test that the pass 
ends up somewhere in the LLVM-created part of the pass pipeline - the parts 
that you can't get to from the way the original pass addition was written in 
Clang. At least I assume that's the case/what motivated the change from adding 
it in Clang to adding it in LLVM?

eg: if LLVM always forms passes {x, y, z} and Clang is able to add passes 
before/after, say it always adds 'a' before and 'b' after, to make {a, x, y, z, 
b} - and this new pass u was previously added at the start to make {u, a, x, y, 
z, b} but now needs to go in {a, x, y, u, z, b} you could test that 'u' is 
after 'a' and before 'b', or between 'x' and 'z', etc. If there's some other 
more clear/simple/reliable marker of where the LLVM-created passes start/end in 
the structured dump, that'd be good to use as a landmark to make such a test 
more robust. If there's some meaningful pass that this pass always needs to go 
after - testing that might be OK, even if it's somewhat an implementation 
detail of LLVM - whatever's likely to make the test more legible and more 
reliable/resilient to unrelated changes would be good.

> As you pointed out, the test here is to test if the specific pass is run and 
> gives expected results.

If that's the case, this test could be committed standalone, before any of 
these other changes?

> The pipeline ordering could be tested with a LLVM test but that would require 
> a LLVM switch setup for UniqueLinkageNames and I'm not sure there's a need 
> for that switch except for testing.

That's OK, the entire 'opt' tool and all its switches only exist for testing. 
eg: 
https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

dblaikie wrote:
> Does this test validate the new behavior? (ie: does this test fail without 
> the LLVM changes and pass with it) Not that it necessarily has to - since 
> Clang isn't here to test the LLVM behavior - perhaps this test is sufficient 
> in Clang to test that the code in BackendUtil works to enable this pass.
> 
> This could possibly be staged as independent commits - adding the LLVM 
> functionality in one commit, which would be a no-op for Clang because it 
> wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang change 
> that would remove the custom pass addition and set PTO.UniqueLinkageNames - 
> and then it'd probably be reasonable to have this test be made a bit more 
> explicit (testing the pass manager structure/order) to show that that Clang 
> change had an effect: Moving the pass to the desired location in the pass 
> pipeline.
This is a good question. No, this test does not validate the pipeline change on 
the LLVM side, since Clang shouldn't have knowledge about how the pipelines are 
arranged in LLVM. As you pointed out, the test here is to test if the specific 
pass is run and gives expected results.

Thanks for the suggestion to break the Clang changes and LLVM changes apart 
which would make the testing more specific. The pipeline ordering could be 
tested with a LLVM test but that would require a LLVM switch setup for 
UniqueLinkageNames and I'm not sure there's a need for that switch except for 
testing.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:1-2
+; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+

dblaikie wrote:
> Does this test test any changes that are made in the rest of the patch? Since 
> the test is specifying the pass to test, I would've assumed this test would 
> pass with or without any changes that might move that pass around in the pass 
> order (or indeed add or remove it from the pass manager in general).
> 
> I'd expect this change to be tested in LLVM by a pass manager structure test, 
> similar to the Clang test.
Right, the test here does not test the specific pass order set up by this 
change. It tests that the ordering change does not revert what's already there, 
i.e, the `UniqueInternalLinkageNamesPass` is still run and gives expected 
results.

Ideally the pipeline ordering should also be tested but that would require a 
LLVM switch setup to enable the pass and I'm not sure there's a need for that 
switch except for testing.


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global

Does this test validate the new behavior? (ie: does this test fail without the 
LLVM changes and pass with it) Not that it necessarily has to - since Clang 
isn't here to test the LLVM behavior - perhaps this test is sufficient in Clang 
to test that the code in BackendUtil works to enable this pass.

This could possibly be staged as independent commits - adding the LLVM 
functionality in one commit, which would be a no-op for Clang because it 
wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang change 
that would remove the custom pass addition and set PTO.UniqueLinkageNames - and 
then it'd probably be reasonable to have this test be made a bit more explicit 
(testing the pass manager structure/order) to show that that Clang change had 
an effect: Moving the pass to the desired location in the pass pipeline.



Comment at: 
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:1-2
+; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+

Does this test test any changes that are made in the rest of the patch? Since 
the test is specifying the pass to test, I would've assumed this test would 
pass with or without any changes that might move that pass around in the pass 
order (or indeed add or remove it from the pass manager in general).

I'd expect this change to be tested in LLVM by a pass manager structure test, 
similar to the Clang test.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313566.
hoy edited the summary of this revision.
hoy added a comment.

Checking pipeline in clang test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,37 @@
+; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0, !dbg !18
+}
+
+define dso_local i32 (...)* @bar() !dbg !7 {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*), !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, null}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 10, column: 3, scope: !7)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DILocation(line: 6, column: 3, scope: !15)
+
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -284,6 +284,7 @@
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
   CallGraphProfile = true;
   MergeFunctions = false;
+  UniqueLinkageNames = false;
 }
 
 extern cl::opt EnableConstraintElimination;
@@ -1001,6 +1002,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1770,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -127,6 +127,9 @@
   /// Tuning option to enable/disable function merging. Its default value is
   /// false.
   bool MergeFunctions;
+
+  /// Uniquefy function linkage name. Its default value is false.
+  bool UniqueLinkageNames;
 };
 
 /// This class provides access to building LLVM's passes.
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,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++ -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 -

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2469504 , @dblaikie wrote:

> In D93656#2469485 , @hoy wrote:
>
>> In D93656#2468841 , @aeubanks wrote:
>>
>>> In D93656#2468834 , @hoy wrote:
>>>
 In D93656#2468821 , @aeubanks 
 wrote:

> Also it looks like this is doing 2 different things, the moving of things 
> from Clang to LLVM's PassBuilder, and separately the change to the pass 
> itself. Can these be separated?

 I'm not sure about a good way to separate them. There are Clang tests that 
 may fail with removing the pass from clang while not adding it 
 correspondingly in llvm. Adding the pass in llvm while not removing it 
 from Clang may cause the pass to run twice which may also fail the Clang 
 tests. What do you think?
>>>
>>> I mean keep that in one change, but separate out the change to 
>>> llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and 
>>> DebugInfoMetadata.h.
>>
>> I see, thanks for the clarification.
>>
>> In D93656#2468698 , @dblaikie wrote:
>>
>>> This should have a LLVM test coverage for the LLVM changes. (I realize 
>>> they're also tested by the Clang test, because there's no way to test 
>>> Clang's pass manager creation short of testing the effect of running the 
>>> pass manager (hmm - /maybe/ there's a way to dump the pass pipeline? In 
>>> which case that's how Clang should be tested, just testing that it creates 
>>> the right pipeline, not that that pipeline does any particular thing))
>>
>> Added an IR test. There is the llvm switch `-debug-pass=` that can dump the 
>> pass pipeline. I'm not aware of a clang switch that can do that.
>
> Seems some clang tests use something like that, eg:
>
>   clang/test/CodeGen/thinlto-debug-pm.c:// RUN: %clang_cc1 -triple 
> x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o 
> -fthinlto-index=%t.thinlto.bc -fno-experimental-new-pass-manager -mllvm 
> -debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O0-OLDPM

Thanks for pointing that out. It seems that only works with the legacy pass 
manager when `-emit-llvm` is specified. With a quick search in the clang 
regression tests it looks like the pipeline dumps are rarely checked there. 
Such checks are done quite a bit in llvm tests though. Does the current clang 
test look okay to you, or do you prefer checking the pipeline dump there? 
Thanks.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

The changes that rename debug linkage name has been separated as D93747 
. I'm moving the discussion there.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313488.
hoy added a comment.

Fixing IR test failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,36 @@
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0, !dbg !18
+}
+
+define dso_local i32 (...)* @bar() !dbg !7 {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*), !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, null}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 10, column: 3, scope: !7)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DILocation(line: 6, column: 3, scope: !15)
+
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -1001,6 +1001,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1769,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -127,6 +127,9 @@
   /// Tuning option to enable/disable function merging. Its default value is
   /// false.
   bool MergeFunctions;
+
+  /// Uniquefy function linkage name. Its default value is false.
+  bool UniqueLinkageNames;
 };
 
 /// This class provides access to building LLVM's passes.
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1144,6 +1144,7 @@
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
+  PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(CodeGenOpts.DebugPassManager);
@@ -1325,11 +1326,6 @@
   MPM = PB.buildPerModuleDefaultPipeline(Level);
 }
 
-// Add UniqueInternalLinkageNames Pass which renames internal linkage
-// symbols with unique names.
-if (CodeGenOpts.UniqueInternalLinkageNames)
-  MPM.addPass(UniqueInternalLinkageNamesPass());
-
 if (!CodeGenOpts.MemoryProfileOutput.empty()) {
   MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
   MPM.addPass(ModuleMemProfilerPass());
___
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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D93656#2469485 , @hoy wrote:

> In D93656#2468841 , @aeubanks wrote:
>
>> In D93656#2468834 , @hoy wrote:
>>
>>> In D93656#2468821 , @aeubanks 
>>> wrote:
>>>
 Also it looks like this is doing 2 different things, the moving of things 
 from Clang to LLVM's PassBuilder, and separately the change to the pass 
 itself. Can these be separated?
>>>
>>> I'm not sure about a good way to separate them. There are Clang tests that 
>>> may fail with removing the pass from clang while not adding it 
>>> correspondingly in llvm. Adding the pass in llvm while not removing it from 
>>> Clang may cause the pass to run twice which may also fail the Clang tests. 
>>> What do you think?
>>
>> I mean keep that in one change, but separate out the change to 
>> llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and 
>> DebugInfoMetadata.h.
>
> I see, thanks for the clarification.
>
> In D93656#2468698 , @dblaikie wrote:
>
>> This should have a LLVM test coverage for the LLVM changes. (I realize 
>> they're also tested by the Clang test, because there's no way to test 
>> Clang's pass manager creation short of testing the effect of running the 
>> pass manager (hmm - /maybe/ there's a way to dump the pass pipeline? In 
>> which case that's how Clang should be tested, just testing that it creates 
>> the right pipeline, not that that pipeline does any particular thing))
>
> Added an IR test. There is the llvm switch `-debug-pass=` that can dump the 
> pass pipeline. I'm not aware of a clang switch that can do that.

Seems some clang tests use something like that, eg:

  clang/test/CodeGen/thinlto-debug-pm.c:// RUN: %clang_cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o 
-fthinlto-index=%t.thinlto.bc -fno-experimental-new-pass-manager -mllvm 
-debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O0-OLDPM


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313487.
hoy added a comment.

Separated PassBuilder changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,36 @@
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0, !dbg !18
+}
+
+define dso_local i32 (...)* @bar() !dbg !7 {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*), !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, null}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 10, column: 3, scope: !7)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DILocation(line: 6, column: 3, scope: !15)
+
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}() [[ATTR:#[0-9]+]] !dbg ![[#DBG:]]
+; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -1001,6 +1001,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1769,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (PTO.UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -127,6 +127,9 @@
   /// Tuning option to enable/disable function merging. Its default value is
   /// false.
   bool MergeFunctions;
+
+  /// Uniquefy function linkage name. Its default value is false.
+  bool UniqueLinkageNames;
 };
 
 /// This class provides access to building LLVM's passes.
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -1144,6 +1144,7 @@
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
+  PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(CodeGenOpts.DebugPassManager);
@@ -1325,11 +1326,6 @@
   MPM = PB.buildPerModuleDefaultPipeline(Level);
 }
 
-// Add UniqueInternalLinkageNames Pass which renames internal linkage
-// symbols with unique names.
-if (CodeGenOpts.UniqueInternalLinkageNames)
-  MPM.addPass(UniqueInternalLinkageNamesPass());
-
 if (!CodeGenOpts.MemoryProfileOutput.empty()) {
   MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
   MPM.addPass(ModuleMemProfilerPass());
___
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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2468841 , @aeubanks wrote:

> In D93656#2468834 , @hoy wrote:
>
>> In D93656#2468821 , @aeubanks wrote:
>>
>>> Also it looks like this is doing 2 different things, the moving of things 
>>> from Clang to LLVM's PassBuilder, and separately the change to the pass 
>>> itself. Can these be separated?
>>
>> I'm not sure about a good way to separate them. There are Clang tests that 
>> may fail with removing the pass from clang while not adding it 
>> correspondingly in llvm. Adding the pass in llvm while not removing it from 
>> Clang may cause the pass to run twice which may also fail the Clang tests. 
>> What do you think?
>
> I mean keep that in one change, but separate out the change to 
> llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and 
> DebugInfoMetadata.h.

I see, thanks for the clarification.

In D93656#2468698 , @dblaikie wrote:

> This should have a LLVM test coverage for the LLVM changes. (I realize 
> they're also tested by the Clang test, because there's no way to test Clang's 
> pass manager creation short of testing the effect of running the pass manager 
> (hmm - /maybe/ there's a way to dump the pass pipeline? In which case that's 
> how Clang should be tested, just testing that it creates the right pipeline, 
> not that that pipeline does any particular thing))

Added an IR test. There is the llvm switch `-debug-pass=` that can dump the 
pass pipeline. I'm not aware of a clang switch that can do that.


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



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

hoy wrote:
> tmsriram wrote:
> > 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.
> Yeah, it would be nice to leverage an existing API like this but 
> unfortunately I haven't found any.
> 
> Regarding passes that duplicate functions, I think the renaming done by 
> `UniqueInternalLinkageNamesPass` is transparent to subsequent passes, since 
> it is placed very early in the pipeline before any inlining or cloning passes.
Not super radical, no - but more "we haven't had a need for this so far, I 
wonder how other similar use cases (if there are any) deal with this situation 
without this API addition"

> Regarding passes that duplicate functions, I think the renaming done by 
> UniqueInternalLinkageNamesPass is transparent to subsequent passes, since it 
> is placed very early in the pipeline before any inlining or cloning passes.

Sorry, I wasn't meaning to ask "how does this pass interact with those other 
passes" but "how do those other passes deal with debug info, and can we learn 
anything from them/use similar techniques here that they use in their 
implementations"



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:48
+  if (UniqueDebugAndProfileNames) {
+F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+if (DISubprogram *SP = F.getSubprogram()) {

hoy wrote:
> dblaikie wrote:
> > What's this about/for? Should this be in an independent change?
> This is about how the sample profile loader deals with renamed functions. The 
> sample loader will use the original function name (i.e, by ignoring all 
> suffixes appended) to retrieve a profile by default. With the attribute set 
> here. the sample loader will only ignore certain suffixes not including the 
> one (.__uniq.) appended here. Please see `getCanonicalFnName` in 
> `SampleProf.h` for details. The reason that names are uniquefied here for 
> AutoFDO is to have the sample loader differentiate same-named static 
> functions, so the change is sort of related here.
> 
> I'll put more comments for that.  
Sounds like an independent patch, perhaps? (ie: Adding this attribute is 
relevant even if we weren't fixing the debug info name? And fixing the debug 
info name would be relevant even if we weren't adding this attribute) It'd be 
good for it to be in a separate patch to clarify the intent, test coverage, etc.



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

tmsriram wrote:
> 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. 
Best guess, yes.

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


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.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D93656#2468834 , @hoy wrote:

> In D93656#2468821 , @aeubanks wrote:
>
>> Also it looks like this is doing 2 different things, the moving of things 
>> from Clang to LLVM's PassBuilder, and separately the change to the pass 
>> itself. Can these be separated?
>
> I'm not sure about a good way to separate them. There are Clang tests that 
> may fail with removing the pass from clang while not adding it 
> correspondingly in llvm. Adding the pass in llvm while not removing it from 
> Clang may cause the pass to run twice which may also fail the Clang tests. 
> What do you think?

I mean keep that in one change, but separate out the change to 
llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and 
DebugInfoMetadata.h.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2468821 , @aeubanks wrote:

> Also it looks like this is doing 2 different things, the moving of things 
> from Clang to LLVM's PassBuilder, and separately the change to the pass 
> itself. Can these be separated?

I'm not sure about a good way to separate them. There are Clang tests that may 
fail with removing the pass from clang while not adding it correspondingly in 
llvm. Adding the pass in llvm while not removing it from Clang may cause the 
pass to run twice which may also fail the Clang tests. What do you think?




Comment at: llvm/include/llvm/Passes/PassBuilder.h:140
   bool DebugLogging;
+  bool UniqueLinkageNames;
   TargetMachine *TM;

aeubanks wrote:
> This seems better suited to be part of PipelineTuningOptions rather than 
> directly in PassBuilder
Sounds good. Will move it into PipelineTuningOptions.



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"));
+

tmsriram wrote:
> 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.
Sounds good, will remove 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.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Also it looks like this is doing 2 different things, the moving of things from 
Clang to LLVM's PassBuilder, and separately the change to the pass itself. Can 
these be separated?


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.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/include/llvm/Passes/PassBuilder.h:140
   bool DebugLogging;
+  bool UniqueLinkageNames;
   TargetMachine *TM;

This seems better suited to be part of PipelineTuningOptions rather than 
directly in PassBuilder


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

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



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

tmsriram wrote:
> 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.
Yeah, it would be nice to leverage an existing API like this but unfortunately 
I haven't found any.

Regarding passes that duplicate functions, I think the renaming done by 
`UniqueInternalLinkageNamesPass` is transparent to subsequent passes, since it 
is placed very early in the pipeline before any inlining or cloning passes.



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"));
+

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?



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:48
+  if (UniqueDebugAndProfileNames) {
+F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+if (DISubprogram *SP = F.getSubprogram()) {

dblaikie wrote:
> What's this about/for? Should this be in an independent change?
This is about how the sample profile loader deals with renamed functions. The 
sample loader will use the original function name (i.e, by ignoring all 
suffixes appended) to retrieve a profile by default. With the attribute set 
here. the sample loader will only ignore certain suffixes not including the one 
(.__uniq.) appended here. Please see `getCanonicalFnName` in `SampleProf.h` for 
details. The reason that names are uniquefied here for AutoFDO is to have the 
sample loader differentiate same-named static functions, so the change is sort 
of related here.

I'll put more comments for that.  


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.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: rnk.
dblaikie added a comment.

This should have a LLVM test coverage for the LLVM changes. (I realize they're 
also tested by the Clang test, because there's no way to test Clang's pass 
manager creation short of testing the effect of running the pass manager (hmm - 
/maybe/ there's a way to dump the pass pipeline? In which case that's how Clang 
should be tested, just testing that it creates the right pipeline, not that 
that pipeline does any particular thing))

In D93656#2466689 , @tmsriram wrote:

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

@rnk - any thoughts on this?




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

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.



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"));
+

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?



Comment at: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp:48
+  if (UniqueDebugAndProfileNames) {
+F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+if (DISubprogram *SP = F.getSubprogram()) {

What's this about/for? Should this be in an independent change?


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313187.
hoy marked an inline comment as done.
hoy added a comment.

Addressing feedbacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,13 +13,20 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt UniqueDebugAndProfileNames(
+"unqiue-debug-profile-names", cl::init(true), cl::Hidden,
+cl::desc("Uniqueify debug and profile symbol Names"));
+
 static bool uniqueifyInternalLinkageNames(Module &M) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
@@ -31,11 +38,19 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto &F : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (UniqueDebugAndProfileNames) {
+F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+if (DISubprogram *SP = F.getSubprogram()) {
+  auto *Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+}
+  }
   Changed = true;
 }
   }
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -434,8 +434,10 @@
 
 PassBuilder::PassBuilder(bool DebugLogging, TargetMachine *TM,
  PipelineTuningOptions PTO, Optional PGOOpt,
- PassInstrumentationCallbacks *PIC)
-: DebugLogging(DebugLogging), TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
+ PassInstrumentationCallbacks *PIC,
+ bool UniqueLinkageNames)
+: DebugLogging(DebugLogging), UniqueLinkageNames(UniqueLinkageNames),
+  TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
   if (TM)
 TM->registerPassBuilderCallbacks(*this, DebugLogging);
   if (PIC && shouldPopulateClassToPassNames()) {
@@ -1001,6 +1003,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1771,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -137,6 +137,7 @@
 /// construction.
 class PassBuilder {
   bool DebugLogging;
+  bool UniqueLinkageNames;
   TargetMachine *TM;
   PipelineTuningOptions PTO;
   Optional PGOOpt;
@@ -281,7 +282,8 @@
   explicit PassBuilder(bool DebugLogging = false, TargetMachine *TM = nullptr,
PipelineTuningOptions PTO = PipelineTuningOptions(),
Optional PGOOpt = None,
-   PassInstrumentationCallbacks *PIC = nullptr);
+   PassInstrumentationCallbacks *PIC = nullptr,
+   bool UniqueLinkageNames = false);
 
   /// Cross register the analysis managers through their proxies.
   ///
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -20

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

[Hongtao Yu via Phabricator via cfe-commits]

hoy marked 2 inline comments as done.
hoy added inline comments.



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

tmsriram wrote:
> Nit, Move the body to DebugInfoMetadata.h itself? 
Done.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2466689 , @tmsriram wrote:

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

Thanks for the information. Yeah it's pretty straightforward hook it up with 
debug info as an IR pass.


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.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added inline comments.



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"));

tmsriram wrote:
> Can we make it true by default?  Atleast the DW_AT_linkage_name must reflect 
> the new name by default IMO.
Sounds good.



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

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?


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] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Hongtao Yu via Phabricator via cfe-commits]

hoy created this revision.
Herald added subscribers: dexonsmith, wenlei, hiraditya.
hoy requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

`UniqueInternalLinkageNamesPass` is useful to CSSPGO, especially when pseudo 
probe is used. It solves naming conflict for static functions which otherwise 
will share a merged profile and likely have a profile quality issue with 
mismatched CFG checksums. Since the pseudo probe instrumentation happens very 
early in the pipeline, I'm moving `UniqueInternalLinkageNamesPass` right before 
it. This is being done only to the new pass manager. In addition, funtions that 
are renamed have their debug linkage name and an AutoFDO attribute updated 
optionally.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93656

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/IR/DebugInfoMetadata.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,13 +13,20 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt UniqueDebugAndProfileNames(
+"unqiue-debug-profile-names", cl::init(false), cl::Hidden,
+cl::desc("Uniqueify debug and profile symbol Names"));
+
 static bool uniqueifyInternalLinkageNames(Module &M) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
@@ -31,11 +38,19 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto &F : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (UniqueDebugAndProfileNames) {
+F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+if (DISubprogram *SP = F.getSubprogram()) {
+  auto Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+}
+  }
   Changed = true;
 }
   }
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -434,8 +434,10 @@
 
 PassBuilder::PassBuilder(bool DebugLogging, TargetMachine *TM,
  PipelineTuningOptions PTO, Optional PGOOpt,
- PassInstrumentationCallbacks *PIC)
-: DebugLogging(DebugLogging), TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
+ PassInstrumentationCallbacks *PIC,
+ bool UniqueLinkageNames)
+: DebugLogging(DebugLogging), UniqueLinkageNames(UniqueLinkageNames),
+  TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
   if (TM)
 TM->registerPassBuilderCallbacks(*this, DebugLogging);
   if (PIC && shouldPopulateClassToPassNames()) {
@@ -1001,6 +1003,11 @@
ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
@@ -1764,6 +1771,11 @@
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
  PGOOpt->Action == PGOOptions::IRUse))
 addPGOInstrPassesForO0(
Index: llvm/lib/IR/DebugInfoMetadata.cpp
===
--- llvm/lib/IR/DebugInfoMetadata.cpp
+++ llvm/lib/IR/DebugInfoMetadata.cpp
@@ -877,6 +877,10 @@
   return F->getSubprogram() == this;
 }
 
+void DISubprogram::replaceRawLinkageName(MDString *LinkageName) {
+  replaceOperandWith(3, LinkageName);
+}
+
 DILexicalBlock *DILexicalBlock::getImpl(LLVMContext &Context, Metadata *Scope,