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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own. Does it improve profile precision?



================
Comment at: clang/test/CodeGen/unique_internal_funcnames.c:1
+// REQUIRES: x86-registered-target
+
----------------
The convention is file-name.c , rather than file_name.c . The latter exists, but is a minority.


================
Comment at: clang/test/CodeGen/unique_internal_funcnames.c:2
+// REQUIRES: x86-registered-target
+
+// RUN: %clang -target x86_64-pc-linux-gnu -S -o - %s | FileCheck %s --check-prefix=PLAIN
----------------
There should be two tests:

* one in test/Driver/funique-internal-funcnames.c, for `-funique-internal-funcnames -fno-unique-internal-funcnames` testing
* the other one should stay here (test/CodeGen), but should be a -S -emit-llvm test for IR output.

We don't normally do driver --> assembly testing.


================
Comment at: clang/test/CodeGen/unique_internal_funcnames.c:3
+
+// RUN: %clang -target x86_64-pc-linux-gnu -S -o - %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang -target x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - %s | FileCheck %s --check-prefix=PLAIN
----------------
`-target x86_64` should just work (generic ELF). This feature is not tied to linux-gnu.


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

https://reviews.llvm.org/D73307