| Message ID | BF230D13CA30DD48930C31D4099330003A42B15F@FMSMSX101.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On 06/05/13 09:18, Iyer, Balaji V wrote: > > >> -----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. > > Hi Jeff, > This email is going to be a bit long, so I apologize for it ahead of time. > > Here is the diff of c-typeck.c (to show that I have removed the unwanted if statement) > > Index: gcc/c/c-typeck.c > =================================================================== > --- gcc/c/c-typeck.c (revision 199672) > +++ gcc/c/c-typeck.c (working copy) > @@ -2942,6 +2942,8 @@ > 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 @@ > 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) > @@ -3207,16 +3198,10 @@ > > 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; > - } > + error_at (input_location, > + "too few arguments to function %qE", function); > + inform_declaration (fundecl); > + return -1; > } > > return error_args ? -1 : (int) parmnum; > ============================================================================= > > > Here is the testcode that I have written that should trigger the "too few arguments to function error" with the appropriate note to say where the function declaration is. > > ---------------------------------- > /* { dg-do compile } */ > /* { dg-options "-fcilkplus" } */ > > /* Test-case contains no array notation but is compiled with -fcilkplus. > It will still print the too few arguments func, thereby saying the > if-statement after the for-loop to check for !flag_enable_cilkplus || > !is_cilkplus_reduce_function (fundecl) is not valid is always taken. */ > > int func (int, int); /* { dg-message "declared here" } */ > > int main (void) > { > int a = 5, b = 2; > return func (a); /* { dg-error "too few arguments to function" } */ > } > -------------------------------- > > Here is the output when I run this: > > bviyer@lakshmi2a:/export/users/gcc-svn/b-trunk-gcc/gcc> ../../install_dir/trunk-install/bin/gcc -fcilkplus test.c > test.c: In function âmainâ: > test.c:14:3: error: too few arguments to function âfuncâ > return func (a); /* { dg-error "too few arguments to function" } */ > ^ > test.c:9:5: note: declared here > int func (int, int); /* { dg-message "declared here" } */ > ^ > > > > The whole patch with testcode is attached, so is this Ok for trunk? > > Here are the ChangeLog entries: > gcc/c/ChangeLog > 2013-06-05 Balaji V. Iyer <balaji.v.iyer@intel.com> > > * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus > reduction functions outside the for-loop. Added a check if the fundecl > is non-NULL. Finally, removed an unwanted if-statement, and made the > body unconditional > > gcc/testsuite/ChangeLog > 2013-06-05 Balaji V. Iyer <balaji.v.iyer@intel.com> > > PR C/57457 > * c-c++-common/cilk-plus/AN/pr57457.c: New test. > * c-c++-common/cilk-plus/AN/pr57457-2.c: Likewise. This is fine. Please install. Thanks! jeff
Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 199672) +++ gcc/c/c-typeck.c (working copy) @@ -2942,6 +2942,8 @@ 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 @@ 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) @@ -3207,16 +3198,10 @@ 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; - } + error_at (input_location, + "too few arguments to function %qE", function); + inform_declaration (fundecl); + return -1; } return error_args ? -1 : (int) parmnum;