* [PATCH] Fix Cilk+ ICEs in the alias oracle
@ 2014-02-13 11:13 Richard Biener
2014-02-13 12:48 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2014-02-13 11:13 UTC (permalink / raw)
To: gcc-patches; +Cc: balaji.v.iyer
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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Cilk+ ICEs in the alias oracle
2014-02-13 11:13 [PATCH] Fix Cilk+ ICEs in the alias oracle Richard Biener
@ 2014-02-13 12:48 ` Richard Biener
2014-02-14 17:34 ` Jeff Law
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2014-02-13 12:48 UTC (permalink / raw)
To: gcc-patches; +Cc: balaji.v.iyer
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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix Cilk+ ICEs in the alias oracle
2014-02-13 12:48 ` Richard Biener
@ 2014-02-14 17:34 ` Jeff Law
2014-02-14 19:03 ` Iyer, Balaji V
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Law @ 2014-02-14 17:34 UTC (permalink / raw)
To: Richard Biener, gcc-patches; +Cc: balaji.v.iyer
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Fix Cilk+ ICEs in the alias oracle
2014-02-14 17:34 ` Jeff Law
@ 2014-02-14 19:03 ` Iyer, Balaji V
2014-02-15 10:04 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Iyer, Balaji V @ 2014-02-14 19:03 UTC (permalink / raw)
To: Jeff Law, Richard Biener, gcc-patches
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Fix Cilk+ ICEs in the alias oracle
2014-02-14 19:03 ` Iyer, Balaji V
@ 2014-02-15 10:04 ` Richard Biener
0 siblings, 0 replies; 5+ messages in thread
From: Richard Biener @ 2014-02-15 10:04 UTC (permalink / raw)
To: Iyer, Balaji V; +Cc: Jeff Law, gcc-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-15 10:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-13 11:13 [PATCH] Fix Cilk+ ICEs in the alias oracle Richard Biener
2014-02-13 12:48 ` Richard Biener
2014-02-14 17:34 ` Jeff Law
2014-02-14 19:03 ` Iyer, Balaji V
2014-02-15 10:04 ` 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).