| Message ID | BF230D13CA30DD48930C31D4099330003A42ACA4@FMSMSX101.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On 06/04/2013 11:37 AM, Iyer, Balaji V wrote: > > Yes, that does simplify the whole thing. Here is an updated ChangeLog > and patch (with testcode) attached. So, is it Ok for trunk? > > gcc/testsuite/ChangeLog 2013-06-04 Balaji V. Iyer > <balaji.v.iyer@intel.com> > > PR C/57457 * c-c++-common/cilk-plus/AN/pr57457.c: New test. > > gcc/c/ChangeLog 2013-06-04 Balaji V. Iyer > <balaji.v.iyer@intel.com> > > * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus > reduction functions outside the for-loop. Also, added a check if > the fundecl is non-NULL. OK. This looks good. Please install. As a follow-up, don't we need to do something with the test after the loop as well: if (typetail != 0 && TREE_VALUE (typetail) != void_type_node) { /* If array notation is used and Cilk Plus is enabled, then we do not worry about this error now. We will handle them in a later place. */ if (!flag_enable_cilkplus || !is_cilkplus_reduce_builtin (fundecl)) { error_at (input_location, "too few arguments to function %qE", function); inform_declaration (fundecl); return -1; } } ISTM we need to check for a NULL FUNDECL here too. jeff
> -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Tuesday, June 04, 2013 2:07 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Steve Ellcey > Subject: Re: [PATCH] pr57457 > > On 06/04/2013 11:37 AM, Iyer, Balaji V wrote: > > > > Yes, that does simplify the whole thing. Here is an updated ChangeLog > > and patch (with testcode) attached. So, is it Ok for trunk? > > > > gcc/testsuite/ChangeLog 2013-06-04 Balaji V. Iyer > > <balaji.v.iyer@intel.com> > > > > PR C/57457 * c-c++-common/cilk-plus/AN/pr57457.c: New test. > > > > gcc/c/ChangeLog 2013-06-04 Balaji V. Iyer <balaji.v.iyer@intel.com> > > > > * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus > > reduction functions outside the for-loop. Also, added a check if the > > fundecl is non-NULL. > OK. This looks good. Please install. > > As a follow-up, don't we need to do something with the test after the loop as > well: > > if (typetail != 0 && TREE_VALUE (typetail) != void_type_node) > { > /* If array notation is used and Cilk Plus is enabled, then we do not > worry about this error now. We will handle them in a later place. */ > if (!flag_enable_cilkplus > || !is_cilkplus_reduce_builtin (fundecl)) > { > error_at (input_location, > "too few arguments to function %qE", function); > inform_declaration (fundecl); > return -1; > } > } > > ISTM we need to check for a NULL FUNDECL here too. Actually, you can eliminate the entire if-statement (i.e. remove if-statement and make the body unconditional). This is because, if flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl) is true, then it would have returned vec_safe_length(values) and will not even get to this point in the first place. So, this is technically equivalent to if (1). So, can I remove that and check it in also? It passes all my regression tests. Thanks, Balaji V. Iyer. > > jeff
On 06/04/13 12:58, Iyer, Balaji V wrote: > > > Actually, you can eliminate the entire if-statement (i.e. remove > if-statement and make the body unconditional). This is because, if > flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl) > is true, then it would have returned vec_safe_length(values) and will > not even get to this point in the first place. So, this is > technically equivalent to if (1). So, can I remove that and check it > in also? It passes all my regression tests. I originally thought it could be eliminated as well, but after further reflection I couldn't convince myself it'd do the right thing for the case when flag_enable_cilkplus is true but is_cilkplus_reduce_builtin was false. Note that triggering that code my be nontrivial, AFAICT we're suppressing a diagnostic. So you're going to need to write invalid code to get into that condition at the bottom of the loop at all. jeff
> -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Tuesday, June 04, 2013 11:49 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Steve Ellcey > Subject: Re: [PATCH] pr57457 > > On 06/04/13 12:58, Iyer, Balaji V wrote: > > > > > > Actually, you can eliminate the entire if-statement (i.e. remove > > if-statement and make the body unconditional). This is because, if > > flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl) > > is true, then it would have returned vec_safe_length(values) and will > > not even get to this point in the first place. So, this is technically > > equivalent to if (1). So, can I remove that and check it in also? It > > passes all my regression tests. > I originally thought it could be eliminated as well, but after further reflection I > couldn't convince myself it'd do the right thing for the case when > flag_enable_cilkplus is true but is_cilkplus_reduce_builtin was false. > > Note that triggering that code my be nontrivial, AFAICT we're suppressing a > diagnostic. So you're going to need to write invalid code to get into that > condition at the bottom of the loop at all. OK, I will create one tomorrow morning. Thanks, Balaji V. Iyer. > > jeff
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index e5e1455..91ce67a 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -2942,6 +2942,8 @@ convert_arguments (tree typelist, vec<tree, va_gc> *values, break; } } + if (flag_enable_cilkplus && fundecl && is_cilkplus_reduce_builtin (fundecl)) + return vec_safe_length (values); /* Scan the given expressions and types, producing individual converted arguments. */ @@ -2959,17 +2961,6 @@ convert_arguments (tree typelist, vec<tree, va_gc> *values, bool npc; tree parmval; - // FIXME: I assume this code is here to handle the overloaded - // behavior of the __sec_reduce* builtins, and avoid giving - // argument mismatch warnings/errors. We should probably handle - // this with the resolve_overloaded_builtin infrastructure. - /* If the function call is a builtin function call, then we do not - worry about it since we break them up into its equivalent later and - we do the appropriate checks there. */ - if (flag_enable_cilkplus - && is_cilkplus_reduce_builtin (fundecl)) - continue; - if (type == void_type_node) { if (selector) diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c new file mode 100644 index 0000000..68a1fd8 --- /dev/null +++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c @@ -0,0 +1,39 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +/* This test has no array notation components in it and thus should compile + fine without crashing. */ + +typedef unsigned int size_t; +typedef int (*__compar_fn_t) (const void *, const void *); +extern void *bsearch (const void *__key, const void *__base, + size_t __nmemb, size_t __size, __compar_fn_t + __compar) + __attribute__ ((__nonnull__ (1, 2, 5))) ; +extern __inline __attribute__ ((__gnu_inline__)) void * +bsearch (const void *__key, const void *__base, size_t __nmemb, size_t + __size, + __compar_fn_t __compar) +{ + size_t __l, __u, __idx; + const void *__p; + int __comparison; + __l = 0; + __u = __nmemb; + while (__l < __u) + { + __idx = (__l + __u) / 2; + __p = (void *) (((const char *) __base) + + (__idx * __size)); + __comparison = (*__compar) (__key, + __p); + if (__comparison < 0) + __u = __idx; + else if (__comparison > 0) + __l = __idx + 1; + else + return (void *) + __p; + } + return ((void *)0); +}