[PATCH] inline: do not inline when no_profile_instrument_function is different

[PATCH] inline: do not inline when no_profile_instrument_function is different

[Martin Liška]

Hello.

Similarly to e.g. sanitizer attributes, we sould prevent inlining when one function
is marked as not instrumented. We should do that with -fprofile-generate only.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

Adds test-case for:
	PR gcov-profile/80223

gcc/ChangeLog:

	* ipa-inline.c (can_inline_edge_p): Similarly to sanitizer
	options, do not inline when no_profile_instrument_function
	attributes are different.

gcc/testsuite/ChangeLog:

	* gcc.dg/no_profile_instrument_function-attr-2.c: New test.
---
  gcc/ipa-inline.c                                  | 10 ++++++++++
  .../no_profile_instrument_function-attr-2.c       | 15 +++++++++++++++
  2 files changed, 25 insertions(+)
  create mode 100644 gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c

diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 9d896beb2ac..786ab2e7f7f 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -396,6 +396,16 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
        e->inline_failed = CIF_SANITIZE_ATTRIBUTE_MISMATCH;
        inlinable = false;
      }
+  else if (profile_arc_flag
+	   && lookup_attribute ("no_profile_instrument_function",
+				DECL_ATTRIBUTES (caller->decl))
+	      != lookup_attribute ("no_profile_instrument_function",
+				   DECL_ATTRIBUTES (callee->decl)))
+    {
+      e->inline_failed = CIF_UNSPECIFIED;
+      inlinable = false;
+    }
+
    if (!inlinable && report)
      report_inline_failed_reason (e);
    return inlinable;
diff --git a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
new file mode 100644
index 00000000000..586962a1c76
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
@@ -0,0 +1,15 @@
+/* { dg-require-effective-target global_constructor } */
+/* { dg-options "-O2 -fprofile-generate -fprofile-update=single -fdump-tree-optimized" } */
+
+__attribute__ ((no_profile_instrument_function))
+int foo()
+{
+  return 0;
+}
+
+int bar()
+{
+  return foo();
+}
+
+/* { dg-final { scan-tree-dump-times " = foo \\(\\)" 1 "optimized"} } */
-- 
2.32.0

Re: [PATCH] inline: do not inline when no_profile_instrument_function is different

[Jan Hubicka]

> Hello.
> 
> Similarly to e.g. sanitizer attributes, we sould prevent inlining when one function
> is marked as not instrumented. We should do that with -fprofile-generate only.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> Adds test-case for:
> 	PR gcov-profile/80223
> 
> gcc/ChangeLog:
> 
> 	* ipa-inline.c (can_inline_edge_p): Similarly to sanitizer
> 	options, do not inline when no_profile_instrument_function
> 	attributes are different.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/no_profile_instrument_function-attr-2.c: New test.
> ---
>  gcc/ipa-inline.c                                  | 10 ++++++++++
>  .../no_profile_instrument_function-attr-2.c       | 15 +++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
> 
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index 9d896beb2ac..786ab2e7f7f 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -396,6 +396,16 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
>        e->inline_failed = CIF_SANITIZE_ATTRIBUTE_MISMATCH;
>        inlinable = false;
>      }
> +  else if (profile_arc_flag
> +	   && lookup_attribute ("no_profile_instrument_function",
> +				DECL_ATTRIBUTES (caller->decl))
> +	      != lookup_attribute ("no_profile_instrument_function",
> +				   DECL_ATTRIBUTES (callee->decl)))
> +    {
> +      e->inline_failed = CIF_UNSPECIFIED;
> +      inlinable = false;
> +    }

Is there reason to prevent the inlining once instrumentation is done?
I think you can just block it for early inliner.

Honza
> +
>    if (!inlinable && report)
>      report_inline_failed_reason (e);
>    return inlinable;
> diff --git a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
> new file mode 100644
> index 00000000000..586962a1c76
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-require-effective-target global_constructor } */
> +/* { dg-options "-O2 -fprofile-generate -fprofile-update=single -fdump-tree-optimized" } */
> +
> +__attribute__ ((no_profile_instrument_function))
> +int foo()
> +{
> +  return 0;
> +}
> +
> +int bar()
> +{
> +  return foo();
> +}
> +
> +/* { dg-final { scan-tree-dump-times " = foo \\(\\)" 1 "optimized"} } */
> -- 
> 2.32.0
> 

Re: [PATCH] inline: do not inline when no_profile_instrument_function is different

[Martin Liška]

On 6/23/21 2:38 PM, Jan Hubicka wrote:
> Is there reason to prevent the inlining once instrumentation is done?

No ;)

> I think you can just block it for early inliner.

Sure. Do you have a handy predicate function that tells if einliner is done?

Thanks,
Martin

Re: [PATCH] inline: do not inline when no_profile_instrument_function is different

[Nick Desaulniers]

On Wed, Jun 23, 2021 at 6:15 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 6/23/21 2:38 PM, Jan Hubicka wrote:
> > Is there reason to prevent the inlining once instrumentation is done?
>
> No ;)

Here's another case that coincidentally came up yesterday. How should
these attributes behave in the case of __attribute__((flatten)) on the
caller?

Another case which is curious is what happens when the callee has
__attribute__((always_inline)) and there's a conflict?

In LLVM, the current behavior is:
"__attribute__((no_stack_protector)) and
__attribute__((no_profile_instrument_function)) will prevent inline
substitution when the attributes do not match between caller and
callee, unless the callee was attributed with
__attribute__((always_inline)) or the caller was attributed with
__attribute__((flatten))."

We have some overzealous users of always_inline and flatten in
Android. They are currently playing whack-a-mole with
no_stack_protector.  I'm not sure yet how we can better help them self
diagnose, or whether we should consider a change in policy.

I'm also not sure whether GCC's einliner corresponds with
always_inline or not necessarily?
--
Thanks,
~Nick Desaulniers

Re: [PATCH] inline: do not inline when no_profile_instrument_function is different

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On 6/23/21 3:15 PM, Martin Liška wrote:
> On 6/23/21 2:38 PM, Jan Hubicka wrote:
>> Is there reason to prevent the inlining once instrumentation is done?
> 
> No ;)
> 
>> I think you can just block it for early inliner.
> 
> Sure. Do you have a handy predicate function that tells if einliner is done?
> 
> Thanks,
> Martin

Hello.

There's updated version of that patch that blocks inlining only during einline IPA pass.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-inline-do-not-einline-when-no_profile_instrument_fun.patch --]
[-- Type: text/x-patch, Size: 2395 bytes --]

From e2adaff9ed92bcc380e1368418da5ad2801fef4e Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 22 Jun 2021 10:09:01 +0200
Subject: [PATCH] inline: do not einline when no_profile_instrument_function is
 different

	PR gcov-profile/80223

gcc/ChangeLog:

	* ipa-inline.c (can_inline_edge_p): Similarly to sanitizer
	options, do not inline when no_profile_instrument_function
	attributes are different in early inliner. It's fine to inline
	it after PGO instrumentation.

gcc/testsuite/ChangeLog:

	* gcc.dg/no_profile_instrument_function-attr-2.c: New test.
---
 gcc/ipa-inline.c                                | 17 +++++++++++++++++
 .../no_profile_instrument_function-attr-2.c     | 15 +++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c

diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 413446bcc46..012b326b5e9 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -396,6 +396,23 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
       e->inline_failed = CIF_SANITIZE_ATTRIBUTE_MISMATCH;
       inlinable = false;
     }
+  else if (profile_arc_flag
+	   && (lookup_attribute ("no_profile_instrument_function",
+				 DECL_ATTRIBUTES (caller->decl)) == NULL_TREE)
+	   != (lookup_attribute ("no_profile_instrument_function",
+				 DECL_ATTRIBUTES (callee->decl)) == NULL_TREE))
+    {
+      cgraph_node *origin = caller;
+      while (origin->clone_of)
+	origin = origin->clone_of;
+
+      if (!DECL_STRUCT_FUNCTION (origin->decl)->always_inline_functions_inlined)
+	{
+	  e->inline_failed = CIF_UNSPECIFIED;
+	  inlinable = false;
+	}
+    }
+
   if (!inlinable && report)
     report_inline_failed_reason (e);
   return inlinable;
diff --git a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
new file mode 100644
index 00000000000..472eca88efd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
@@ -0,0 +1,15 @@
+/* { dg-require-effective-target global_constructor } */
+/* { dg-options "-O2 -fprofile-generate -fprofile-update=single -fdump-tree-optimized" } */
+
+__attribute__ ((no_profile_instrument_function))
+int foo()
+{
+  return 0;
+}
+
+int bar()
+{
+  return foo();
+}
+
+/* { dg-final { scan-tree-dump-not" = foo \\(\\)" "optimized"} } */
-- 
2.32.0

Re: [PATCH] inline: do not inline when no_profile_instrument_function is different

[Jeff Law]

On 8/18/2021 2:38 AM, Martin Liška wrote:
> On 6/23/21 3:15 PM, Martin Liška wrote:
>> On 6/23/21 2:38 PM, Jan Hubicka wrote:
>>> Is there reason to prevent the inlining once instrumentation is done?
>>
>> No ;)
>>
>>> I think you can just block it for early inliner.
>>
>> Sure. Do you have a handy predicate function that tells if einliner 
>> is done?
>>
>> Thanks,
>> Martin
>
> Hello.
>
> There's updated version of that patch that blocks inlining only during 
> einline IPA pass.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> 0001-inline-do-not-einline-when-no_profile_instrument_fun.patch
>
>  From e2adaff9ed92bcc380e1368418da5ad2801fef4e Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Tue, 22 Jun 2021 10:09:01 +0200
> Subject: [PATCH] inline: do not einline when no_profile_instrument_function is
>   different
>
> 	PR gcov-profile/80223
>
> gcc/ChangeLog:
>
> 	* ipa-inline.c (can_inline_edge_p): Similarly to sanitizer
> 	options, do not inline when no_profile_instrument_function
> 	attributes are different in early inliner. It's fine to inline
> 	it after PGO instrumentation.
OK
jeff

[Committed] Fix fatal typo in gcc.dg/no_profile_instrument_function-attr-2.c

[Hans-Peter Nilsson]

(Committed as obvious.)

Dejagnu is unfortunately brittle: a syntax error in a
directive can abort the test-run for the current "tool"
(gcc, g++, gfortran), and if you don't check for this
condition or actually read the stdout log yourself, your
tools may make you believe the test was successful without
regressions.  At the very least, always grep for ^ERROR: in
the stdout log!

With r12-3379, the testsuite got such a fatal syntax error,
causing the gcc test-run to abort at (e.g.):

...
FAIL: gcc.dg/memchr.c (test for excess errors)
FAIL: gcc.dg/memcmp-3.c (test for excess errors)
ERROR: (DejaGnu) proc "scan-tree-dump-not\" = foo {\(\)"} optimized" does not exist.
The error code is TCL LOOKUP COMMAND scan-tree-dump-not\"
The info on the error is:
invalid command name "scan-tree-dump-not""
    while executing
"::tcl_unknown scan-tree-dump-not\" = foo {\(\)"} optimized"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::tcl_unknown $args"

		=== gcc Summary ===

# of expected passes		63740
# of unexpected failures	38
# of unexpected successes	2
# of expected failures		351
# of unresolved testcases	3
# of unsupported tests		662
x/cris-elf/gccobj/gcc/xgcc  version 12.0.0 20210907 (experimental)\
 [master r12-3391-g849d5f5929fc] (GCC)

testsuite:
	* gcc.dg/no_profile_instrument_function-attr-2.c: Fix
	typo in last change.
---
 gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
index 472eca88efdd..2e93ee5f6891 100644
--- a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
+++ b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-2.c
@@ -12,4 +12,4 @@ int bar()
   return foo();
 }
 
-/* { dg-final { scan-tree-dump-not" = foo \\(\\)" "optimized"} } */
+/* { dg-final { scan-tree-dump-not " = foo \\(\\)" "optimized"} } */
-- 
2.11.0

Re: [Committed] Fix fatal typo in gcc.dg/no_profile_instrument_function-attr-2.c

[Martin Liška]

On 9/7/21 22:41, Hans-Peter Nilsson wrote:
> With r12-3379, the testsuite got such a fatal syntax error,
> causing the gcc test-run to abort at (e.g.):

Thank you for the fix! I haven't noticed the problem as I only grep for '^FAIL'
lines in the corresponding log files.

Cheers,
Martin

Re: [Committed] Fix fatal typo in gcc.dg/no_profile_instrument_function-attr-2.c

[Hans-Peter Nilsson]

> From: Martin Liaka <mliska@suse.cz>
> Date: Wed, 8 Sep 2021 09:29:52 +0200

> On 9/7/21 22:41, Hans-Peter Nilsson wrote:
> > With r12-3379, the testsuite got such a fatal syntax error,
> > causing the gcc test-run to abort at (e.g.):
> 
> Thank you for the fix! I haven't noticed the problem as I only grep for '^FAIL'
> lines in the corresponding log files.

You're welcome but *please* also add (conceptually)
"grep -q ^ERROR: makelog && fail" to your workflow.

brgds, H-P
PS. yeah, but worth repeating

Re: [Committed] Fix fatal typo in gcc.dg/no_profile_instrument_function-attr-2.c

[Rainer Orth]

Hi Hans-Peter,

>> From: Martin Liaka <mliska@suse.cz>
>> Date: Wed, 8 Sep 2021 09:29:52 +0200
>
>> On 9/7/21 22:41, Hans-Peter Nilsson wrote:
>> > With r12-3379, the testsuite got such a fatal syntax error,
>> > causing the gcc test-run to abort at (e.g.):
>> 
>> Thank you for the fix! I haven't noticed the problem as I only grep for '^FAIL'
>> lines in the corresponding log files.
>
> You're welcome but *please* also add (conceptually)
> "grep -q ^ERROR: makelog && fail" to your workflow.

even that is not enough, though: one also needs to look for UNRESOLVED
and XPASS.  An easy way of doing all this is running make
mail-report.log after the build and comparing the output to an
unmodified build.  Saves everyone loads of trouble.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University