[PATCH] Fix Cilk+ ICEs in the alias oracle

[PATCH] Fix Cilk+ ICEs in the alias oracle

[Richard Biener]

Cilk+ builds INDIRECT_REFs when expanding builtins (oops) and thus
those can leak into MEM_EXRs which will lead to ICEs later.
The following patch properly builds a MEM_REF instead.  Grepping
for INDIRECT_REF I found another suspicious use (just removed,
it cannot have triggered and it looks bogus) and the use of
a langhook instead of proper GIMPLE interfaces (function also
used during expansion).

Bootstrap / testing in progress together with some other stuff.

Ok?

Thanks,
Richard.

2014-02-13  Richard Biener  <rguenther@suse.de>

	* cilk-common.c: Include gimple-expr.h.
	(cilk_arrow): Build a MEM_REF, not an INDIRECT_REF.
	(get_frame_arg): Use middel-end types_compatible_p.  Do not
	strip INDIRECT_REFs.

Index: gcc/cilk-common.c
===================================================================
--- gcc/cilk-common.c	(revision 207725)
+++ gcc/cilk-common.c	(working copy)
@@ -32,6 +32,7 @@ along with GCC; see the file COPYING3.
 #include "recog.h"
 #include "tree-iterator.h"
 #include "gimplify.h"
+#include "gimple-expr.h"
 #include "cilk.h"
 
 /* This structure holds all the important fields of the internal structures,
@@ -66,8 +67,7 @@ cilk_dot (tree frame, int field_number,
 tree
 cilk_arrow (tree frame_ptr, int field_number, bool volatil)
 {
-  return cilk_dot (fold_build1 (INDIRECT_REF, 
-				TREE_TYPE (TREE_TYPE (frame_ptr)), frame_ptr), 
+  return cilk_dot (build_simple_mem_ref (frame_ptr), 
 		   field_number, volatil);
 }
 
@@ -287,12 +287,11 @@ get_frame_arg (tree call)
 
   argtype = TREE_TYPE (argtype);
   
-  gcc_assert (!lang_hooks.types_compatible_p
-	      || lang_hooks.types_compatible_p (argtype, cilk_frame_type_decl));
+  gcc_assert (types_compatible_p (argtype, cilk_frame_type_decl));
 
   /* If it is passed in as an address, then just use the value directly 
      since the function is inlined.  */
-  if (TREE_CODE (arg) == INDIRECT_REF || TREE_CODE (arg) == ADDR_EXPR)
+  if (TREE_CODE (arg) == ADDR_EXPR)
     return TREE_OPERAND (arg, 0);
   return arg;
 }

Re: [PATCH] Fix Cilk+ ICEs in the alias oracle

[Richard Biener]

On Thu, 13 Feb 2014, Richard Biener wrote:

> 
> Cilk+ builds INDIRECT_REFs when expanding builtins (oops) and thus
> those can leak into MEM_EXRs which will lead to ICEs later.
> The following patch properly builds a MEM_REF instead.  Grepping
> for INDIRECT_REF I found another suspicious use (just removed,
> it cannot have triggered and it looks bogus) and the use of
> a langhook instead of proper GIMPLE interfaces (function also
> used during expansion).
> 
> Bootstrap / testing in progress together with some other stuff.
> 
> Ok?

Btw, this exposes that Cilk+ is LTO-ignorant - it doesn't properly
register its global trees (bah, more global trees...).  So
the types_compatible_p call ICEs.  Trying to process them in
lto/lto.c:read_cgraph_and_symbols doesn't seem to work though.

So I'm opting to remove the assert and leave fixing LTO for
somebody who cares about Cilk+.

Simpifies the patch as follows, bootstrapped & tested on
x86_64-unknown-linux-gnu.

Richard.

2014-02-13  Richard Biener  <rguenther@suse.de>

	* cilk-common.c (cilk_arrow): Build a MEM_REF, not an INDIRECT_REF.
	(get_frame_arg): Drop the assert with langhook types_compatible_p.
	Do not strip INDIRECT_REFs.

Index: gcc/cilk-common.c
===================================================================
--- gcc/cilk-common.c	(revision 207725)
+++ gcc/cilk-common.c	(working copy)
@@ -66,8 +66,7 @@ cilk_dot (tree frame, int field_number,
 tree
 cilk_arrow (tree frame_ptr, int field_number, bool volatil)
 {
-  return cilk_dot (fold_build1 (INDIRECT_REF, 
-				TREE_TYPE (TREE_TYPE (frame_ptr)), frame_ptr), 
+  return cilk_dot (build_simple_mem_ref (frame_ptr), 
 		   field_number, volatil);
 }
 
@@ -287,12 +286,9 @@ get_frame_arg (tree call)
 
   argtype = TREE_TYPE (argtype);
   
-  gcc_assert (!lang_hooks.types_compatible_p
-	      || lang_hooks.types_compatible_p (argtype, cilk_frame_type_decl));
-
   /* If it is passed in as an address, then just use the value directly 
      since the function is inlined.  */
-  if (TREE_CODE (arg) == INDIRECT_REF || TREE_CODE (arg) == ADDR_EXPR)
+  if (TREE_CODE (arg) == ADDR_EXPR)
     return TREE_OPERAND (arg, 0);
   return arg;
 }

Re: [PATCH] Fix Cilk+ ICEs in the alias oracle

[Jeff Law]

On 02/13/14 05:47, Richard Biener wrote:
> On Thu, 13 Feb 2014, Richard Biener wrote:
>
>>
>> Cilk+ builds INDIRECT_REFs when expanding builtins (oops) and thus
>> those can leak into MEM_EXRs which will lead to ICEs later.
>> The following patch properly builds a MEM_REF instead.  Grepping
>> for INDIRECT_REF I found another suspicious use (just removed,
>> it cannot have triggered and it looks bogus) and the use of
>> a langhook instead of proper GIMPLE interfaces (function also
>> used during expansion).
>>
>> Bootstrap / testing in progress together with some other stuff.
>>
>> Ok?
>
> Btw, this exposes that Cilk+ is LTO-ignorant - it doesn't properly
> register its global trees (bah, more global trees...).  So
> the types_compatible_p call ICEs.  Trying to process them in
> lto/lto.c:read_cgraph_and_symbols doesn't seem to work though.
>
> So I'm opting to remove the assert and leave fixing LTO for
> somebody who cares about Cilk+.
>
> Simpifies the patch as follows, bootstrapped & tested on
> x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2014-02-13  Richard Biener  <rguenther@suse.de>
>
> 	* cilk-common.c (cilk_arrow): Build a MEM_REF, not an INDIRECT_REF.
> 	(get_frame_arg): Drop the assert with langhook types_compatible_p.
> 	Do not strip INDIRECT_REFs.
FWIW, I see a recurring issue here.  Specifically I'm regularly seeing 
cases where submissions are not playing well with LTO.   Speaking 
strictly for myself, I'm not LTO-aware enough to spot them in patches as 
they fly by.

It's not meant to be a criticism, just noting a recurring issue.

jeff

RE: [PATCH] Fix Cilk+ ICEs in the alias oracle

[Iyer, Balaji V]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Friday, February 14, 2014 12:34 PM
> To: Richard Biener; gcc-patches@gcc.gnu.org
> Cc: Iyer, Balaji V
> Subject: Re: [PATCH] Fix Cilk+ ICEs in the alias oracle
> 
> On 02/13/14 05:47, Richard Biener wrote:
> > On Thu, 13 Feb 2014, Richard Biener wrote:
> >
> >>
> >> Cilk+ builds INDIRECT_REFs when expanding builtins (oops) and thus
> >> those can leak into MEM_EXRs which will lead to ICEs later.
> >> The following patch properly builds a MEM_REF instead.  Grepping for
> >> INDIRECT_REF I found another suspicious use (just removed, it cannot
> >> have triggered and it looks bogus) and the use of a langhook instead
> >> of proper GIMPLE interfaces (function also used during expansion).
> >>
> >> Bootstrap / testing in progress together with some other stuff.
> >>
> >> Ok?
> >
> > Btw, this exposes that Cilk+ is LTO-ignorant - it doesn't properly
> > register its global trees (bah, more global trees...).  So the
> > types_compatible_p call ICEs.  Trying to process them in
> > lto/lto.c:read_cgraph_and_symbols doesn't seem to work though.
> >
> > So I'm opting to remove the assert and leave fixing LTO for somebody
> > who cares about Cilk+.
> >
> > Simpifies the patch as follows, bootstrapped & tested on
> > x86_64-unknown-linux-gnu.
> >
> > Richard.
> >
> > 2014-02-13  Richard Biener  <rguenther@suse.de>
> >
> > 	* cilk-common.c (cilk_arrow): Build a MEM_REF, not an
> INDIRECT_REF.
> > 	(get_frame_arg): Drop the assert with langhook
> types_compatible_p.
> > 	Do not strip INDIRECT_REFs.
> FWIW, I see a recurring issue here.  Specifically I'm regularly seeing
> cases where submissions are not playing well with LTO.   Speaking
> strictly for myself, I'm not LTO-aware enough to spot them in patches as they
> fly by.

I thought I had handled LTO correctly. I apologize if I made a mistake. I assure you that it was not deliberate. I even had my tests use -flto flags to make sure it is going through it correctly...

> 
> It's not meant to be a criticism, just noting a recurring issue.
> 
> jeff

RE: [PATCH] Fix Cilk+ ICEs in the alias oracle

[Richard Biener]

On Fri, 14 Feb 2014, Iyer, Balaji V wrote:

> 
> 
> > -----Original Message-----
> > From: Jeff Law [mailto:law@redhat.com]
> > Sent: Friday, February 14, 2014 12:34 PM
> > To: Richard Biener; gcc-patches@gcc.gnu.org
> > Cc: Iyer, Balaji V
> > Subject: Re: [PATCH] Fix Cilk+ ICEs in the alias oracle
> > 
> > On 02/13/14 05:47, Richard Biener wrote:
> > > On Thu, 13 Feb 2014, Richard Biener wrote:
> > >
> > >>
> > >> Cilk+ builds INDIRECT_REFs when expanding builtins (oops) and thus
> > >> those can leak into MEM_EXRs which will lead to ICEs later.
> > >> The following patch properly builds a MEM_REF instead.  Grepping for
> > >> INDIRECT_REF I found another suspicious use (just removed, it cannot
> > >> have triggered and it looks bogus) and the use of a langhook instead
> > >> of proper GIMPLE interfaces (function also used during expansion).
> > >>
> > >> Bootstrap / testing in progress together with some other stuff.
> > >>
> > >> Ok?
> > >
> > > Btw, this exposes that Cilk+ is LTO-ignorant - it doesn't properly
> > > register its global trees (bah, more global trees...).  So the
> > > types_compatible_p call ICEs.  Trying to process them in
> > > lto/lto.c:read_cgraph_and_symbols doesn't seem to work though.
> > >
> > > So I'm opting to remove the assert and leave fixing LTO for somebody
> > > who cares about Cilk+.
> > >
> > > Simpifies the patch as follows, bootstrapped & tested on
> > > x86_64-unknown-linux-gnu.
> > >
> > > Richard.
> > >
> > > 2014-02-13  Richard Biener  <rguenther@suse.de>
> > >
> > > 	* cilk-common.c (cilk_arrow): Build a MEM_REF, not an
> > INDIRECT_REF.
> > > 	(get_frame_arg): Drop the assert with langhook
> > types_compatible_p.
> > > 	Do not strip INDIRECT_REFs.
> > FWIW, I see a recurring issue here.  Specifically I'm regularly seeing
> > cases where submissions are not playing well with LTO.   Speaking
> > strictly for myself, I'm not LTO-aware enough to spot them in patches as they
> > fly by.
> 
> I thought I had handled LTO correctly. I apologize if I made a mistake. 
> I assure you that it was not deliberate. I even had my tests use -flto 
> flags to make sure it is going through it correctly...

By using the langhook types_compatible_p you by-passed the failure
on LTO (because that langhook is not implemented there).

As it's only builtins expansion the mismatches don't really matter.

Richard.