Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Martin Liška]

On 03/15/2017 10:30 AM, Rainer Orth wrote:

Hi Martin,


This is a small follow-up patch, where local.local flag should be also dropped
for a default implementation.

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

Ready to be installed?


the testcase causes

WARNING: profopt.exp does not support dg-error

I suspect it can just as well go into gcc.dg with


Thanks for pointing out. As I read the test-case, the dg-error is not needed.
I'm going to install following patch as obvious.

Martin



/* { dg-require-profiling "-fprofile-generate" } */

and -fprofile-generate added to dg-options.

Rainer



>From 3ac3656787ded319f19c347a3576474adc48a6d9 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 15 Mar 2017 11:03:27 +0100
Subject: [PATCH] Removed unused dg-error.

gcc/testsuite/ChangeLog:

2017-03-15  Martin Liska  

	* gcc.dg/tree-prof/pr66295.c: Removed unused dg-error.
---
 gcc/testsuite/gcc.dg/tree-prof/pr66295.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/tree-prof/pr66295.c b/gcc/testsuite/gcc.dg/tree-prof/pr66295.c
index 1ab7e6c8f64..b90ef84ed10 100644
--- a/gcc/testsuite/gcc.dg/tree-prof/pr66295.c
+++ b/gcc/testsuite/gcc.dg/tree-prof/pr66295.c
@@ -11,7 +11,7 @@ foo (double *__restrict a, double *__restrict b, int n)
 }
 
 double
-bar (double *__restrict a, double *__restrict b, int n)	/* { dg-error "attribute\[^\n\r]*foo\[^\n\r]* is unknown" } */
+bar (double *__restrict a, double *__restrict b, int n)
 {
   double s;
   int i;
-- 
2.11.1

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Rainer Orth]

Hi Martin,

> This is a small follow-up patch, where local.local flag should be also dropped
> for a default implementation.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

the testcase causes

WARNING: profopt.exp does not support dg-error

I suspect it can just as well go into gcc.dg with

/* { dg-require-profiling "-fprofile-generate" } */

and -fprofile-generate added to dg-options.

Rainer

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

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Richard Biener]

On Tue, Mar 14, 2017 at 12:04 PM, Martin Liška  wrote:
> On 03/14/2017 11:29 AM, Richard Biener wrote:
>> On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška  wrote:
>>> On 03/14/2017 10:46 AM, Richard Biener wrote:
 On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška  wrote:
> Hello.
>
> This is a small follow-up patch, where local.local flag should be also 
> dropped
> for a default implementation.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

 I see we have clered the flag on the clones this way.  But isn't it
 bogus to have
 it set in the first place?  That is, isn't analysis sofar given bogus info?
>>>
>>> Yes, I did it for clones. Reason why we mark it is that local flag is set
>>> by pass_ipa_function_and_variable_visibility pass, which runs before MV 
>>> pass.
>>>
>>> I can imagine MV can bail out for a non-trivial reason and the visibility 
>>> pass
>>> should somehow simulate and predict what happens in the MV pass?
>>
>> Setting .local to true is an optimization, isn't it?  So just not doing it 
>> when
>> the attribute is present should be safe.  Just "undoing" .local = true later
>> is asking for trouble if some intermediate pass decides to do some transform
>> based on .local = true, no?
>
> Ok, there's more detail analysis. When a MV function is set local (either 
> it's a static symbol),
> or IPA split (or IPA inline) makes a clone. This is all fine because we 
> define local as:
>
> struct GTY(()) cgraph_local_info {
>   /* Set when function is visible in current compilation unit only and
>  its address is never taken.  */
>   unsigned local : 1;
>
> All IPA passes can happily makes optimization based on that. However, in 
> moment where we
> introduce a dispatcher (that obviously takes an address), then the function 
> starts to not
> be local.
>
> Thus I believe my original patch (and the part which is already in trunk) 
> should work
> in the right way.

Ah, ok.  I thought it was also supposed to preserve the ABI but if
only the ABI of the
dispatcher (the original fndecl) matters then this is moot.

Patch is ok then.

Thanks,
Richard.

> Martin
>
>>
>> Richard.
>>
>>> Martin
>>>

 Shouldn't we instead fix local_p to not mark functions with MV attribute 
 local
 in the first place?

 Richard.

> Martin
>>>
>

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Martin Liška]

On 03/14/2017 11:29 AM, Richard Biener wrote:
> On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška  wrote:
>> On 03/14/2017 10:46 AM, Richard Biener wrote:
>>> On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška  wrote:
 Hello.

 This is a small follow-up patch, where local.local flag should be also 
 dropped
 for a default implementation.

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

 Ready to be installed?
>>>
>>> I see we have clered the flag on the clones this way.  But isn't it
>>> bogus to have
>>> it set in the first place?  That is, isn't analysis sofar given bogus info?
>>
>> Yes, I did it for clones. Reason why we mark it is that local flag is set
>> by pass_ipa_function_and_variable_visibility pass, which runs before MV pass.
>>
>> I can imagine MV can bail out for a non-trivial reason and the visibility 
>> pass
>> should somehow simulate and predict what happens in the MV pass?
> 
> Setting .local to true is an optimization, isn't it?  So just not doing it 
> when
> the attribute is present should be safe.  Just "undoing" .local = true later
> is asking for trouble if some intermediate pass decides to do some transform
> based on .local = true, no?

Ok, there's more detail analysis. When a MV function is set local (either it's 
a static symbol),
or IPA split (or IPA inline) makes a clone. This is all fine because we define 
local as:

struct GTY(()) cgraph_local_info {
  /* Set when function is visible in current compilation unit only and
 its address is never taken.  */
  unsigned local : 1;

All IPA passes can happily makes optimization based on that. However, in moment 
where we
introduce a dispatcher (that obviously takes an address), then the function 
starts to not
be local.

Thus I believe my original patch (and the part which is already in trunk) 
should work
in the right way.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Shouldn't we instead fix local_p to not mark functions with MV attribute 
>>> local
>>> in the first place?
>>>
>>> Richard.
>>>
 Martin
>>

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Richard Biener]

On Tue, Mar 14, 2017 at 11:09 AM, Martin Liška  wrote:
> On 03/14/2017 10:46 AM, Richard Biener wrote:
>> On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška  wrote:
>>> Hello.
>>>
>>> This is a small follow-up patch, where local.local flag should be also 
>>> dropped
>>> for a default implementation.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>
>> I see we have clered the flag on the clones this way.  But isn't it
>> bogus to have
>> it set in the first place?  That is, isn't analysis sofar given bogus info?
>
> Yes, I did it for clones. Reason why we mark it is that local flag is set
> by pass_ipa_function_and_variable_visibility pass, which runs before MV pass.
>
> I can imagine MV can bail out for a non-trivial reason and the visibility pass
> should somehow simulate and predict what happens in the MV pass?

Setting .local to true is an optimization, isn't it?  So just not doing it when
the attribute is present should be safe.  Just "undoing" .local = true later
is asking for trouble if some intermediate pass decides to do some transform
based on .local = true, no?

Richard.

> Martin
>
>>
>> Shouldn't we instead fix local_p to not mark functions with MV attribute 
>> local
>> in the first place?
>>
>> Richard.
>>
>>> Martin
>

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Martin Liška]

On 03/14/2017 10:46 AM, Richard Biener wrote:
> On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška  wrote:
>> Hello.
>>
>> This is a small follow-up patch, where local.local flag should be also 
>> dropped
>> for a default implementation.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> I see we have clered the flag on the clones this way.  But isn't it
> bogus to have
> it set in the first place?  That is, isn't analysis sofar given bogus info?

Yes, I did it for clones. Reason why we mark it is that local flag is set
by pass_ipa_function_and_variable_visibility pass, which runs before MV pass.

I can imagine MV can bail out for a non-trivial reason and the visibility pass
should somehow simulate and predict what happens in the MV pass?

Martin

> 
> Shouldn't we instead fix local_p to not mark functions with MV attribute local
> in the first place?
> 
> Richard.
> 
>> Martin

Re: [PATCH 2] Fix multiple target clones nodes (PR lto/66295).

[Richard Biener]

On Mon, Mar 13, 2017 at 4:19 PM, Martin Liška  wrote:
> Hello.
>
> This is a small follow-up patch, where local.local flag should be also dropped
> for a default implementation.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

I see we have clered the flag on the clones this way.  But isn't it
bogus to have
it set in the first place?  That is, isn't analysis sofar given bogus info?

Shouldn't we instead fix local_p to not mark functions with MV attribute local
in the first place?

Richard.

> Martin