[patch] Fix PR fortran/72743

[patch] Fix PR fortran/72743

[Chung-Lin Tang]

[-- 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

Re: [patch] Fix PR fortran/72743

[Richard Biener]

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)

Re: [patch] Fix PR fortran/72743

[Chung-Lin Tang]

[-- 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;
     }
 }

Re: [patch] Fix PR fortran/72743

[Richard Biener]

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)