Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

Hi.

Honza can you please take a look at this, because Richi installed patch
that I originally suggested and you were not happy about: r251333.

Thanks,
Martin

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

On 08/10/2017 03:39 PM, Jan Hubicka wrote:
>>>>>>> -  /* If callee has no option attributes, then it is ok to inline.  */
>>>>>>> -  if (!callee_tree)
>>>>>>> +  /* If callee has no option attributes (or default),
>>>>>>> + then it is ok to inline.  */
>>>>>>> +  if (!callee_tree || callee_tree == target_option_default_node)
>>>>>>
>>>>>> I am not sure this actually makes sense, because 
>>>>>> target_option_default_node is not very
>>>>>> meaningful for LTO (it contains whatever was passed to LTO driver). 
>>>>>
>>>>> I see!
>>>>>
>>>>>  Perhaps one can check
>>>>>> for explicit optimization/machine attribute and whether caller and 
>>>>>> callee come from
>>>
>>> I'm not sure what you mean by 'for explicit optimization/machine attribute' 
>>> ?
> 
> You can simply do lookup_attribute and see if callee_tree was set because of 
> attribute
> or because of LTO.  In current patch the comparsion with 
> target_option_default_node
> still does not make much of sense.  But perhaps just 
> lookup_attribute ("optimize", DECL_ATTRIBUTES (...)) would do the job. This 
> is already
> done in ipa-inline.
> 
> Honza

Like this?

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

>>>
>>> I'm attaching a new patch, is it closer?
>>>
>>> Martin
>>>
>>>>>> same compilation unit, though this is quite hackish and will do 
>>>>>> unexpected things with COMDATs.
>>>>>
>>>>> That's quite cumbersome. Any other idea than marking the PR as won't fix?
>>>>
>>>> Yep, it is not prettiest. The problem is that the concept that callee can 
>>>> change semantics
>>>> when no explicit attribute is present is sloppy.  I am not sure how many 
>>>> programs rely on it
>>>> (it is kind of surprising to see functions not being inlined into your 
>>>> target attribute annotated
>>>> function I guess).
>>>> Note that we check for original file in inliner already - this can be done 
>>>> by comparing lto_file_data
>>>> of corresponding cgraph nodes.
>>>>
>>>> Honza
>>>>
>>>

>From 139ac66628dac7835980326fe92aca9eeff5fe5c Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Tue, 27 Jun 2017 16:39:27 +0200
Subject: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR
 target/71991).

gcc/testsuite/ChangeLog:

2017-06-28  Martin Liska  <mli...@suse.cz>

	PR target/71991
	* gcc.dg/torture/pr71991.c: New test.

gcc/ChangeLog:

2017-06-28  Martin Liska  <mli...@suse.cz>

	PR target/71991
	* config/i386/i386.c (ix86_can_inline_p): Make inlining consistent
	in LTO and non-LTO mode.
---
 gcc/config/i386/i386.c |  3 ++-
 gcc/testsuite/gcc.dg/torture/pr71991.c | 12 
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr71991.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b04321a8d40..1bb2a0f39be 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -7507,7 +7507,8 @@ ix86_can_inline_p (tree caller, tree callee)
   tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
 
   /* If callee has no option attributes, then it is ok to inline.  */
-  if (!callee_tree)
+  if (!callee_tree
+  || lookup_attribute ("optimize", DECL_ATTRIBUTES (callee)) == NULL_TREE)
 ret = true;
 
   /* If caller has no option attributes, but callee does then it is not ok to
diff --git a/gcc/testsuite/gcc.dg/torture/pr71991.c b/gcc/testsuite/gcc.dg/torture/pr71991.c
new file mode 100644
index 000..79c927f6844
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71991.c
@@ -0,0 +1,12 @@
+/* PR target/71991 */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-std=c99" } */
+
+static inline int __attribute__ ((__always_inline__)) fn1 () { return 0; }
+static inline int __attribute__ ((target("inline-all-stringops"))) fn2 () { return fn1 (); }
+
+int main()
+{
+  return fn2();
+}
-- 
2.13.3

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Jan Hubicka]

> > -  /* If callee has no option attributes, then it is ok to inline.  */
> > -  if (!callee_tree)
> > +  /* If callee has no option attributes (or default),
> > + then it is ok to inline.  */
> > +  if (!callee_tree || callee_tree == target_option_default_node)
> 
>  I am not sure this actually makes sense, because 
>  target_option_default_node is not very
>  meaningful for LTO (it contains whatever was passed to LTO driver). 
> >>>
> >>> I see!
> >>>
> >>>  Perhaps one can check
>  for explicit optimization/machine attribute and whether caller and 
>  callee come from
> > 
> > I'm not sure what you mean by 'for explicit optimization/machine attribute' 
> > ?

You can simply do lookup_attribute and see if callee_tree was set because of 
attribute
or because of LTO.  In current patch the comparsion with 
target_option_default_node
still does not make much of sense.  But perhaps just 
lookup_attribute ("optimize", DECL_ATTRIBUTES (...)) would do the job. This is 
already
done in ipa-inline.

Honza
> > 
> > I'm attaching a new patch, is it closer?
> > 
> > Martin
> > 
>  same compilation unit, though this is quite hackish and will do 
>  unexpected things with COMDATs.
> >>>
> >>> That's quite cumbersome. Any other idea than marking the PR as won't fix?
> >>
> >> Yep, it is not prettiest. The problem is that the concept that callee can 
> >> change semantics
> >> when no explicit attribute is present is sloppy.  I am not sure how many 
> >> programs rely on it
> >> (it is kind of surprising to see functions not being inlined into your 
> >> target attribute annotated
> >> function I guess).
> >> Note that we check for original file in inliner already - this can be done 
> >> by comparing lto_file_data
> >> of corresponding cgraph nodes.
> >>
> >> Honza
> >>
> > 

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

Honza?

Thanks,
Martin

On 06/30/2017 03:50 PM, Martin Liška wrote:
> On 06/28/2017 05:18 PM, Jan Hubicka wrote:
>>> On 06/28/2017 04:24 PM, Jan Hubicka wrote:
> -  /* If callee has no option attributes, then it is ok to inline.  */
> -  if (!callee_tree)
> +  /* If callee has no option attributes (or default),
> + then it is ok to inline.  */
> +  if (!callee_tree || callee_tree == target_option_default_node)

 I am not sure this actually makes sense, because 
 target_option_default_node is not very
 meaningful for LTO (it contains whatever was passed to LTO driver). 
>>>
>>> I see!
>>>
>>>  Perhaps one can check
 for explicit optimization/machine attribute and whether caller and callee 
 come from
> 
> I'm not sure what you mean by 'for explicit optimization/machine attribute' ?
> 
> I'm attaching a new patch, is it closer?
> 
> Martin
> 
 same compilation unit, though this is quite hackish and will do unexpected 
 things with COMDATs.
>>>
>>> That's quite cumbersome. Any other idea than marking the PR as won't fix?
>>
>> Yep, it is not prettiest. The problem is that the concept that callee can 
>> change semantics
>> when no explicit attribute is present is sloppy.  I am not sure how many 
>> programs rely on it
>> (it is kind of surprising to see functions not being inlined into your 
>> target attribute annotated
>> function I guess).
>> Note that we check for original file in inliner already - this can be done 
>> by comparing lto_file_data
>> of corresponding cgraph nodes.
>>
>> Honza
>>
> 

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

On 06/28/2017 05:18 PM, Jan Hubicka wrote:
>> On 06/28/2017 04:24 PM, Jan Hubicka wrote:
>>>> -  /* If callee has no option attributes, then it is ok to inline.  */
>>>> -  if (!callee_tree)
>>>> +  /* If callee has no option attributes (or default),
>>>> + then it is ok to inline.  */
>>>> +  if (!callee_tree || callee_tree == target_option_default_node)
>>>
>>> I am not sure this actually makes sense, because target_option_default_node 
>>> is not very
>>> meaningful for LTO (it contains whatever was passed to LTO driver). 
>>
>> I see!
>>
>>  Perhaps one can check
>>> for explicit optimization/machine attribute and whether caller and callee 
>>> come from

I'm not sure what you mean by 'for explicit optimization/machine attribute' ?

I'm attaching a new patch, is it closer?

Martin

>>> same compilation unit, though this is quite hackish and will do unexpected 
>>> things with COMDATs.
>>
>> That's quite cumbersome. Any other idea than marking the PR as won't fix?
> 
> Yep, it is not prettiest. The problem is that the concept that callee can 
> change semantics
> when no explicit attribute is present is sloppy.  I am not sure how many 
> programs rely on it
> (it is kind of surprising to see functions not being inlined into your target 
> attribute annotated
> function I guess).
> Note that we check for original file in inliner already - this can be done by 
> comparing lto_file_data
> of corresponding cgraph nodes.
> 
> Honza
> 

>From c07dd3f079382dca617f1a2c32b83f7eaa1797f9 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Tue, 27 Jun 2017 16:39:27 +0200
Subject: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR
 target/71991).

gcc/testsuite/ChangeLog:

2017-06-28  Martin Liska  <mli...@suse.cz>

	PR target/71991
	* gcc.dg/torture/pr71991.c: New test.

gcc/ChangeLog:

2017-06-28  Martin Liska  <mli...@suse.cz>

	PR target/71991
	* config/i386/i386.c (ix86_can_inline_p): Make inlining consistent
	in LTO and non-LTO mode.
---
 gcc/config/i386/i386.c |  9 +++--
 gcc/testsuite/gcc.dg/torture/pr71991.c | 12 
 2 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr71991.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2c4479e1751..92cda94b556 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -7438,8 +7438,13 @@ ix86_can_inline_p (tree caller, tree callee)
   tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
   tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
 
-  /* If callee has no option attributes, then it is ok to inline.  */
-  if (!callee_tree)
+  /* If callee has no option attributes (or default),
+ then it is ok to inline.  */
+  cgraph_node *caller_node = cgraph_node::get (caller);
+  cgraph_node *callee_node = cgraph_node::get (callee);
+  if (!callee_tree
+  || (callee_tree == target_option_default_node
+	  && caller_node->lto_file_data == callee_node->lto_file_data))
 ret = true;
 
   /* If caller has no option attributes, but callee does then it is not ok to
diff --git a/gcc/testsuite/gcc.dg/torture/pr71991.c b/gcc/testsuite/gcc.dg/torture/pr71991.c
new file mode 100644
index 000..79c927f6844
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71991.c
@@ -0,0 +1,12 @@
+/* PR target/71991 */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-std=c99" } */
+
+static inline int __attribute__ ((__always_inline__)) fn1 () { return 0; }
+static inline int __attribute__ ((target("inline-all-stringops"))) fn2 () { return fn1 (); }
+
+int main()
+{
+  return fn2();
+}
-- 
2.13.1

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Jan Hubicka]

> On 06/28/2017 04:24 PM, Jan Hubicka wrote:
> >> -  /* If callee has no option attributes, then it is ok to inline.  */
> >> -  if (!callee_tree)
> >> +  /* If callee has no option attributes (or default),
> >> + then it is ok to inline.  */
> >> +  if (!callee_tree || callee_tree == target_option_default_node)
> > 
> > I am not sure this actually makes sense, because target_option_default_node 
> > is not very
> > meaningful for LTO (it contains whatever was passed to LTO driver). 
> 
> I see!
> 
>  Perhaps one can check
> > for explicit optimization/machine attribute and whether caller and callee 
> > come from
> > same compilation unit, though this is quite hackish and will do unexpected 
> > things with COMDATs.
> 
> That's quite cumbersome. Any other idea than marking the PR as won't fix?

Yep, it is not prettiest. The problem is that the concept that callee can 
change semantics
when no explicit attribute is present is sloppy.  I am not sure how many 
programs rely on it
(it is kind of surprising to see functions not being inlined into your target 
attribute annotated
function I guess).
Note that we check for original file in inliner already - this can be done by 
comparing lto_file_data
of corresponding cgraph nodes.

Honza

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

On 06/28/2017 04:24 PM, Jan Hubicka wrote:
>> -  /* If callee has no option attributes, then it is ok to inline.  */
>> -  if (!callee_tree)
>> +  /* If callee has no option attributes (or default),
>> + then it is ok to inline.  */
>> +  if (!callee_tree || callee_tree == target_option_default_node)
> 
> I am not sure this actually makes sense, because target_option_default_node 
> is not very
> meaningful for LTO (it contains whatever was passed to LTO driver). 

I see!

 Perhaps one can check
> for explicit optimization/machine attribute and whether caller and callee 
> come from
> same compilation unit, though this is quite hackish and will do unexpected 
> things with COMDATs.

That's quite cumbersome. Any other idea than marking the PR as won't fix?

Martin

> 
> honza
>>  ret = true;
>>  
>>/* If caller has no option attributes, but callee does then it is not ok 
>> to
>> diff --git a/gcc/testsuite/gcc.dg/torture/pr71991.c 
>> b/gcc/testsuite/gcc.dg/torture/pr71991.c
>> new file mode 100644
>> index 000..79c927f6844
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/torture/pr71991.c
>> @@ -0,0 +1,12 @@
>> +/* PR target/71991 */
>> +
>> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
>> +/* { dg-options "-std=c99" } */
>> +
>> +static inline int __attribute__ ((__always_inline__)) fn1 () { return 0; }
>> +static inline int __attribute__ ((target("inline-all-stringops"))) fn2 () { 
>> return fn1 (); }
>> +
>> +int main()
>> +{
>> +  return fn2();
>> +}
>>
> 

Re: [PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Jan Hubicka]

> -  /* If callee has no option attributes, then it is ok to inline.  */
> -  if (!callee_tree)
> +  /* If callee has no option attributes (or default),
> + then it is ok to inline.  */
> +  if (!callee_tree || callee_tree == target_option_default_node)

I am not sure this actually makes sense, because target_option_default_node is 
not very
meaningful for LTO (it contains whatever was passed to LTO driver).  Perhaps 
one can check
for explicit optimization/machine attribute and whether caller and callee come 
from
same compilation unit, though this is quite hackish and will do unexpected 
things with COMDATs.

honza
>  ret = true;
>  
>/* If caller has no option attributes, but callee does then it is not ok to
> diff --git a/gcc/testsuite/gcc.dg/torture/pr71991.c 
> b/gcc/testsuite/gcc.dg/torture/pr71991.c
> new file mode 100644
> index 000..79c927f6844
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr71991.c
> @@ -0,0 +1,12 @@
> +/* PR target/71991 */
> +
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-std=c99" } */
> +
> +static inline int __attribute__ ((__always_inline__)) fn1 () { return 0; }
> +static inline int __attribute__ ((target("inline-all-stringops"))) fn2 () { 
> return fn1 (); }
> +
> +int main()
> +{
> +  return fn2();
> +}
> 

[PATCH] Make inlining consistent in LTO and non-LTO mode (PR target/71991).

[Martin Liška]

Hi.

Following patch makes non-LTO and LTO mode to behave same.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
The test-case works on x86_64-linux-gnu.

Ready to be installed?
Martin

gcc/testsuite/ChangeLog:

2017-06-28  Martin Liska  

PR target/71991
* gcc.dg/torture/pr71991.c: New test.

gcc/ChangeLog:

2017-06-28  Martin Liska  

PR target/71991
* config/i386/i386.c (ix86_can_inline_p): Make inlining consistent
in LTO and non-LTO mode.
---
 gcc/config/i386/i386.c |  5 +++--
 gcc/testsuite/gcc.dg/torture/pr71991.c | 12 
 2 files changed, 15 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr71991.c


diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5e8d0ff0e3d..090b93aecfe 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -7438,8 +7438,9 @@ ix86_can_inline_p (tree caller, tree callee)
   tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
   tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
 
-  /* If callee has no option attributes, then it is ok to inline.  */
-  if (!callee_tree)
+  /* If callee has no option attributes (or default),
+ then it is ok to inline.  */
+  if (!callee_tree || callee_tree == target_option_default_node)
 ret = true;
 
   /* If caller has no option attributes, but callee does then it is not ok to
diff --git a/gcc/testsuite/gcc.dg/torture/pr71991.c b/gcc/testsuite/gcc.dg/torture/pr71991.c
new file mode 100644
index 000..79c927f6844
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71991.c
@@ -0,0 +1,12 @@
+/* PR target/71991 */
+
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-std=c99" } */
+
+static inline int __attribute__ ((__always_inline__)) fn1 () { return 0; }
+static inline int __attribute__ ((target("inline-all-stringops"))) fn2 () { return fn1 (); }
+
+int main()
+{
+  return fn2();
+}