[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@davidxl,
I added comments in 
https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2

@MaskRay,
I followed the style guide for nested if statements in 
https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[David Li via Phabricator via cfe-commits]

davidxl added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;

gulfem wrote:
> MaskRay wrote:
> > https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> > 
> > That said, nested if is usually written as a single if with `&&`
> Thanks for the tip @MaskRay, and I'm going to fix that in a following patch. 
It would be helpful to add a comment in the code reflecting the review 
discussions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;

MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> 
> That said, nested if is usually written as a single if with `&&`
Thanks for the tip @MaskRay, and I'm going to fix that in a following patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

That said, nested if is usually written as a single if with `&&`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc7f91e227a79: [InstrProfiling] No runtime hook for unused 
funcs (authored by gulfem).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3406843 , @vsk wrote:

> So long as it doesn't change behavior expected by tapi on Darwin, I think 
> it's OK. I doubt any other platforms are similarly affected.

I don't think it should impact TAPI because it relies on 
`needsRuntimeHookUnconditionally` function, which only returns false for 
`Fuchsia`.
So, this patch only affects the behavior of pulling in profile runtime for 
unused functions in `Fuchsia`.

  bool needsRuntimeHookUnconditionally(const Triple ) {
// On Fuchsia, we only need runtime hook if any counters are present.
if (TT.isOSFuchsia())
  return false;
  
return true;
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

So long as it doesn't change behavior expected by tapi on Darwin, I think it's 
OK. I doubt any other platforms are similarly affected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3406186 , @vsk wrote:

> Sorry for ay delayed replies - I've switched teams at Apple and find it 
> difficult to keep up with llvm reviews.
>
>> it's my understanding is that we might be generating coverage record for 
>> unused functions for TAPI.
>
> Coverage function records are emitted for unused functions because the 
> tooling needs to know which file/line ranges require a "0" execution count.

Thanks for the background @vsk. That means that we need to generate a coverage 
record even for unused functions.
Do you have any objection to the current solution (not pulling in profile 
runtime for such cases)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a subscriber: cishida.
vsk added a comment.

Sorry for ay delayed replies - I've switched teams at Apple and find it 
difficult to keep up with llvm reviews.

> it's my understanding is that we might be generating coverage record for 
> unused functions for TAPI.

Coverage function records are emitted for unused functions because the tooling 
needs to know which file/line ranges require a "0" execution count.
CC'ing @cishida for tapi expertise. My (possibly outdated) understanding is 
that for tapi, we always emit the runtime hook because it analyzes compilation 
flags to determine the expected exported symbol set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3405835 , @davidxl wrote:

> Should this be fixed in Clang (not generating coverage record)? Adding the 
> logic here is a little confusing.

@davidxl,
I originally did that (not generating coverage records) because I think it is a 
cleaner way.
But, after reading https://reviews.llvm.org/D43794, it's my understanding is 
that we might be generating coverage record for unused functions for TAPI.
@vsk can clarify that. If there is no such restriction anymore, I can change 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[David Li via Phabricator via cfe-commits]

davidxl added a comment.

Should this be fixed in Clang (not generating coverage record)? Adding the 
logic here is a little confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 417945.
gulfem added a comment.

Addressed phosek's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Petr Hosek via Phabricator via cfe-commits]

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

LGTM




Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569-573
+if (!CoverageNamesVar) {
+  return MadeChange;
+} else if (!NeedsRuntimeHook) {
+  return MadeChange;
+}

This could be combined into a single statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added subscribers: ellis, abrachet, phosek, hiraditya.
Herald added a project: All.
gulfem requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

CoverageMappingModuleGen generates a coverage mapping record
even for unused functions with internal linkage, e.g.
static int foo() { return 100; }
Clang frontend eliminates such functions, but InstrProfiling pass
still pulls in profile runtime since there is a coverage record.
Fuchsia uses runtime counter relocation, and pulling in profile
runtime for unused functions causes a linker error:
undefined hidden symbol: __llvm_profile_counter_bias.
Since 389dc94d4be7 
, we do 
not hook profile runtime for the binaries
that none of its translation units have been instrumented in Fuchsia.
This patch extends that for the instrumented binaries that
consist of only unused functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,20 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar) {
+  return MadeChange;
+} else if (!NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,20 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar) {
+  return MadeChange;
+} else if (!NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits