* [patch] Fix PR fortran/72743
@ 2016-08-31 10:10 Chung-Lin Tang
2016-09-01 7:14 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Chung-Lin Tang @ 2016-08-31 10:10 UTC (permalink / raw)
To: Richard Biener, Martin Liška; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
Hi Richard, Martin,
this issue is actually sort of like PR 70856, basically the same ICE
after IPA-ICF, due to DECL_PT_UIDs not consistent after reaching for the
ultimate_alias_target().
The reason this wasn't covered by the PR70856 fix is that, in this case,
the DECL_PT_UID was not set in original->decl, evading the condition, and
hence the the merged alias doesn't have DECL_PT_UID set, and causes the
ICE in the second round of IPA-PTA (under -fopenacc).
My fix is to simply remove the DECL_PT_UID_SET_P (original->decl) guard,
and allow the DECL_PT_UID to be set using the DECL_UID in this case.
Does this fix make sense?
Testing does show no regressions, and the PR testcase ICE is fixed.
Thanks,
Chung-Lin
PR fortran/72743
* ipa-icf.c (sem_variable::merge): Remove guard condition for
setting DECL_PT_UID (alias->decl).
testsuite/
* gfortran.dg/goacc/pr72743.f90: New test.
[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 1107 bytes --]
Index: ipa-icf.c
===================================================================
--- ipa-icf.c (revision 239624)
+++ ipa-icf.c (working copy)
@@ -2258,8 +2258,7 @@ sem_variable::merge (sem_item *alias_item)
varpool_node::create_alias (alias_var->decl, decl);
alias->resolve_alias (original);
- if (DECL_PT_UID_SET_P (original->decl))
- SET_DECL_PT_UID (alias->decl, DECL_PT_UID (original->decl));
+ SET_DECL_PT_UID (alias->decl, DECL_PT_UID (original->decl));
if (dump_file)
fprintf (dump_file, "Unified; Variable alias has been created.\n\n");
Index: testsuite/gfortran.dg/goacc/pr72743.f90
===================================================================
--- testsuite/gfortran.dg/goacc/pr72743.f90 (revision 0)
+++ testsuite/gfortran.dg/goacc/pr72743.f90 (revision 0)
@@ -0,0 +1,15 @@
+! { dg-do compile }
+! { dg-additional-options "-O2" }
+
+program p
+ integer, parameter :: n = 8
+ integer :: i, z(n)
+ z = [(i, i=1,n)]
+ print *, z
+end
+subroutine s
+ integer, parameter :: n = 8
+ integer :: i, z(n)
+ z = [(i, i=1,n)]
+ print *, z
+end
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] Fix PR fortran/72743
2016-08-31 10:10 [patch] Fix PR fortran/72743 Chung-Lin Tang
@ 2016-09-01 7:14 ` Richard Biener
2016-09-09 14:33 ` Chung-Lin Tang
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2016-09-01 7:14 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: Martin Liška, gcc-patches
On Wed, 31 Aug 2016, Chung-Lin Tang wrote:
> Hi Richard, Martin,
> this issue is actually sort of like PR 70856, basically the same ICE
> after IPA-ICF, due to DECL_PT_UIDs not consistent after reaching for the
> ultimate_alias_target().
>
> The reason this wasn't covered by the PR70856 fix is that, in this case,
> the DECL_PT_UID was not set in original->decl, evading the condition, and
> hence the the merged alias doesn't have DECL_PT_UID set, and causes the
> ICE in the second round of IPA-PTA (under -fopenacc).
>
> My fix is to simply remove the DECL_PT_UID_SET_P (original->decl) guard,
> and allow the DECL_PT_UID to be set using the DECL_UID in this case.
>
> Does this fix make sense?
> Testing does show no regressions, and the PR testcase ICE is fixed.
If original is the ultimate alias target then this is indeed correct.
The IPA PTA code asserts that all decls that are an alias of the ultimate
alias target X have the same DECL_PT_UID as X DECL_UID (or if the new
alias doesn't have DECL_PT_UID set it will set it that way).
So I'd say it should be
SET_DECL_PT_UID (alias->decl, DECL_UID (original->decl));
instead.
If IPA-ICF would merge
static const int x = 1;
static const int xa __attribute__((alias("x")));
static const int y = 1;
static const int ya __attribute__((alias("y")));
we'd initially have DECL_PT_UID (xa) be DECL_UID (x) and
DECL_PT_UID (ya) be DECL_UID (y). Depending on how exactly IPA-ICF
performs the merging of x and y (and thus adjusts the existing aliases)
IPA-ICF needs to adjust DECL_PT_UID of all aliases of the new
"ultimate alias target". I don't think the current code with the
proposed patch makes sure this will happen.
Thanks,
Richard.
> Thanks,
> Chung-Lin
>
> PR fortran/72743
> * ipa-icf.c (sem_variable::merge): Remove guard condition for
> setting DECL_PT_UID (alias->decl).
>
> testsuite/
> * gfortran.dg/goacc/pr72743.f90: New test.
>
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] Fix PR fortran/72743
2016-09-01 7:14 ` Richard Biener
@ 2016-09-09 14:33 ` Chung-Lin Tang
2016-09-14 9:00 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Chung-Lin Tang @ 2016-09-09 14:33 UTC (permalink / raw)
To: Richard Biener, Chung-Lin Tang; +Cc: Martin Liška, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]
On 2016/9/1 03:13 PM, Richard Biener wrote:
> On Wed, 31 Aug 2016, Chung-Lin Tang wrote:
>
>> Hi Richard, Martin,
>> this issue is actually sort of like PR 70856, basically the same ICE
>> after IPA-ICF, due to DECL_PT_UIDs not consistent after reaching for the
>> ultimate_alias_target().
>>
>> The reason this wasn't covered by the PR70856 fix is that, in this case,
>> the DECL_PT_UID was not set in original->decl, evading the condition, and
>> hence the the merged alias doesn't have DECL_PT_UID set, and causes the
>> ICE in the second round of IPA-PTA (under -fopenacc).
>>
>> My fix is to simply remove the DECL_PT_UID_SET_P (original->decl) guard,
>> and allow the DECL_PT_UID to be set using the DECL_UID in this case.
>>
>> Does this fix make sense?
>> Testing does show no regressions, and the PR testcase ICE is fixed.
>
> If original is the ultimate alias target then this is indeed correct.
>
> The IPA PTA code asserts that all decls that are an alias of the ultimate
> alias target X have the same DECL_PT_UID as X DECL_UID (or if the new
> alias doesn't have DECL_PT_UID set it will set it that way).
>
> So I'd say it should be
>
> SET_DECL_PT_UID (alias->decl, DECL_UID (original->decl));
>
> instead.
>
> If IPA-ICF would merge
>
> static const int x = 1;
> static const int xa __attribute__((alias("x")));
> static const int y = 1;
> static const int ya __attribute__((alias("y")));
>
> we'd initially have DECL_PT_UID (xa) be DECL_UID (x) and
> DECL_PT_UID (ya) be DECL_UID (y). Depending on how exactly IPA-ICF
> performs the merging of x and y (and thus adjusts the existing aliases)
> IPA-ICF needs to adjust DECL_PT_UID of all aliases of the new
> "ultimate alias target". I don't think the current code with the
> proposed patch makes sure this will happen.
>
> Thanks,
> Richard.
How about this fix? The code now sets all DECLs reachable through referring
IPA_REF_ALIAS references. I am not sure of the possibility of cycles,
though it appears there's other code catching alias cycles, issuing an error
to the user.
Again tested without regressions.
Thanks,
Chung-Lin
* ipa-icf.c (set_alias_uids): New function.
(sem_variable::merge): Use set_alias_uids to set DECL_PT_UID of
all the merged variable's referring aliases.
[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 1295 bytes --]
Index: ipa-icf.c
===================================================================
--- ipa-icf.c (revision 239624)
+++ ipa-icf.c (working copy)
@@ -2133,6 +2133,23 @@ sem_variable::get_hash (void)
return m_hash;
}
+/* Set all points-to UIDs of aliases pointing to node N as UID. */
+
+static void
+set_alias_uids (symtab_node *n, int uid)
+{
+ ipa_ref *ref;
+ FOR_EACH_ALIAS (n, ref)
+ {
+ if (dump_file)
+ fprintf (dump_file, " Setting points-to UID of [%s] as %d\n",
+ xstrdup_for_dump (ref->referring->asm_name ()), uid);
+
+ SET_DECL_PT_UID (ref->referring->decl, uid);
+ set_alias_uids (ref->referring, uid);
+ }
+}
+
/* Merges instance with an ALIAS_ITEM, where alias, thunk or redirection can
be applied. */
@@ -2258,12 +2275,11 @@ sem_variable::merge (sem_item *alias_item)
varpool_node::create_alias (alias_var->decl, decl);
alias->resolve_alias (original);
- if (DECL_PT_UID_SET_P (original->decl))
- SET_DECL_PT_UID (alias->decl, DECL_PT_UID (original->decl));
if (dump_file)
- fprintf (dump_file, "Unified; Variable alias has been created.\n\n");
+ fprintf (dump_file, "Unified; Variable alias has been created.\n");
+ set_alias_uids (original, DECL_UID (original->decl));
return true;
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] Fix PR fortran/72743
2016-09-09 14:33 ` Chung-Lin Tang
@ 2016-09-14 9:00 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2016-09-14 9:00 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: Chung-Lin Tang, Martin Liška, gcc-patches
On Fri, 9 Sep 2016, Chung-Lin Tang wrote:
> On 2016/9/1 03:13 PM, Richard Biener wrote:
> > On Wed, 31 Aug 2016, Chung-Lin Tang wrote:
> >
> >> Hi Richard, Martin,
> >> this issue is actually sort of like PR 70856, basically the same ICE
> >> after IPA-ICF, due to DECL_PT_UIDs not consistent after reaching for the
> >> ultimate_alias_target().
> >>
> >> The reason this wasn't covered by the PR70856 fix is that, in this case,
> >> the DECL_PT_UID was not set in original->decl, evading the condition, and
> >> hence the the merged alias doesn't have DECL_PT_UID set, and causes the
> >> ICE in the second round of IPA-PTA (under -fopenacc).
> >>
> >> My fix is to simply remove the DECL_PT_UID_SET_P (original->decl) guard,
> >> and allow the DECL_PT_UID to be set using the DECL_UID in this case.
> >>
> >> Does this fix make sense?
> >> Testing does show no regressions, and the PR testcase ICE is fixed.
> >
> > If original is the ultimate alias target then this is indeed correct.
> >
> > The IPA PTA code asserts that all decls that are an alias of the ultimate
> > alias target X have the same DECL_PT_UID as X DECL_UID (or if the new
> > alias doesn't have DECL_PT_UID set it will set it that way).
> >
> > So I'd say it should be
> >
> > SET_DECL_PT_UID (alias->decl, DECL_UID (original->decl));
> >
> > instead.
> >
> > If IPA-ICF would merge
> >
> > static const int x = 1;
> > static const int xa __attribute__((alias("x")));
> > static const int y = 1;
> > static const int ya __attribute__((alias("y")));
> >
> > we'd initially have DECL_PT_UID (xa) be DECL_UID (x) and
> > DECL_PT_UID (ya) be DECL_UID (y). Depending on how exactly IPA-ICF
> > performs the merging of x and y (and thus adjusts the existing aliases)
> > IPA-ICF needs to adjust DECL_PT_UID of all aliases of the new
> > "ultimate alias target". I don't think the current code with the
> > proposed patch makes sure this will happen.
> >
> > Thanks,
> > Richard.
>
> How about this fix? The code now sets all DECLs reachable through referring
> IPA_REF_ALIAS references. I am not sure of the possibility of cycles,
> though it appears there's other code catching alias cycles, issuing an error
> to the user.
>
> Again tested without regressions.
Looks good to me.
Thanks,
Richard.
> Thanks,
> Chung-Lin
>
> * ipa-icf.c (set_alias_uids): New function.
> (sem_variable::merge): Use set_alias_uids to set DECL_PT_UID of
> all the merged variable's referring aliases.
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-14 8:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 10:10 [patch] Fix PR fortran/72743 Chung-Lin Tang
2016-09-01 7:14 ` Richard Biener
2016-09-09 14:33 ` Chung-Lin Tang
2016-09-14 9:00 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).