RE: [PATCH] Fix for PR c/57563
On Mon, 10 Jun 2013, Iyer, Balaji V wrote: > > This version is better, but if removing an EXCESS_PRECISION_EXPR there > > caused > > problems, why is it OK to remove CONVERT_EXPR and NOP_EXPR like you still > > do - won't that also cause type mismatches (at least if the conversions are > > to > > types that count as sufficiently different for GIMPLE purposes - say > > conversions > > between 32-bit and 64-bit integers)? Maybe you actually need to fold > > without > > removing any such wrappers first at all? > > I looked into it and they were an artifact of previous implementation. > Those while loops were not even being entered. Thus, I took them out. > Here is a fixed patch. Thanks, this patch is OK. -- Joseph S. Myers jos...@codesourcery.com
FW: [PATCH] Fix for PR c/57563
Here is the ChangeLog entries. Sorry I forgot to include in my previous email. gcc/c/ChangeLog 2013-06-10 Balaji V. Iyer * c-array-notation.c (fix_builtin_array_notation_fn): Fully folded excessive precision expressions in function parameters. Also removed couple unwanted while statements. gcc/testsuite/ChangeLog 2013-06-10 Balaji V. Iyer PR c/57563 * c-c++-common/cilk-plus/AN/builtin_fn_mutating.c (main): Fixed a bug in how we check __sec_reduce_mutating function's result.
RE: [PATCH] Fix for PR c/57563
> -Original Message- > From: Joseph Myers [mailto:jos...@codesourcery.com] > Sent: Monday, June 10, 2013 5:18 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek; mpola...@gcc.gnu.org > Subject: RE: [PATCH] Fix for PR c/57563 > > On Mon, 10 Jun 2013, Iyer, Balaji V wrote: > > > I looked into it a bit more detail. It was an error on my side. I was > > removing the excess precision expr layer instead of fully folding it. > > I did that change (i.e. fully fold the expression) and all the errors > > seem to go away. Here is the fixed patch that fixes PR c/57563. It > > passes for > > 32 bit and 64 bit tests. Here are the changelog entries: > > This version is better, but if removing an EXCESS_PRECISION_EXPR there caused > problems, why is it OK to remove CONVERT_EXPR and NOP_EXPR like you still > do - won't that also cause type mismatches (at least if the conversions are to > types that count as sufficiently different for GIMPLE purposes - say > conversions > between 32-bit and 64-bit integers)? Maybe you actually need to fold without > removing any such wrappers first at all? I looked into it and they were an artifact of previous implementation. Those while loops were not even being entered. Thus, I took them out. Here is a fixed patch. Thanks, Balaji V. Iyer. > > -- > Joseph S. Myers > jos...@codesourcery.com diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c old mode 100644 new mode 100755 index b1040da..3285969 --- a/gcc/c/c-array-notation.c +++ b/gcc/c/c-array-notation.c @@ -143,25 +143,18 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MUTATING) { call_fn = CALL_EXPR_ARG (an_builtin_fn, 2); - while (TREE_CODE (call_fn) == CONVERT_EXPR -|| TREE_CODE (call_fn) == NOP_EXPR) + if (TREE_CODE (call_fn) == ADDR_EXPR) call_fn = TREE_OPERAND (call_fn, 0); - call_fn = TREE_OPERAND (call_fn, 0); - identity_value = CALL_EXPR_ARG (an_builtin_fn, 0); - while (TREE_CODE (identity_value) == CONVERT_EXPR -|| TREE_CODE (identity_value) == NOP_EXPR) - identity_value = TREE_OPERAND (identity_value, 0); func_parm = CALL_EXPR_ARG (an_builtin_fn, 1); } else func_parm = CALL_EXPR_ARG (an_builtin_fn, 0); - while (TREE_CODE (func_parm) == CONVERT_EXPR -|| TREE_CODE (func_parm) == EXCESS_PRECISION_EXPR -|| TREE_CODE (func_parm) == NOP_EXPR) -func_parm = TREE_OPERAND (func_parm, 0); - + /* Fully fold any EXCESSIVE_PRECISION EXPR that can occur in the function + parameter. */ + func_parm = c_fully_fold (func_parm, false, NULL); + location = EXPR_LOCATION (an_builtin_fn); if (!find_rank (location, an_builtin_fn, an_builtin_fn, true, &rank)) diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c index 6635565..7c194c2 100644 --- a/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c +++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c @@ -44,11 +44,11 @@ int main(void) max_value = array3[0] * array4[0]; for (ii = 0; ii < 10; ii++) if (array3[ii] * array4[ii] > max_value) { - max_value = array3[ii] * array4[ii]; max_index = ii; } - + for (ii = 0; ii < 10; ii++) +my_func (&max_value, array3[ii] * array4[ii]); #if HAVE_IO for (ii = 0; ii < 10; ii++)
RE: [PATCH] Fix for PR c/57563
On Mon, 10 Jun 2013, Iyer, Balaji V wrote: > I looked into it a bit more detail. It was an error on my side. I was > removing the excess precision expr layer instead of fully folding it. I > did that change (i.e. fully fold the expression) and all the errors seem > to go away. Here is the fixed patch that fixes PR c/57563. It passes for > 32 bit and 64 bit tests. Here are the changelog entries: This version is better, but if removing an EXCESS_PRECISION_EXPR there caused problems, why is it OK to remove CONVERT_EXPR and NOP_EXPR like you still do - won't that also cause type mismatches (at least if the conversions are to types that count as sufficiently different for GIMPLE purposes - say conversions between 32-bit and 64-bit integers)? Maybe you actually need to fold without removing any such wrappers first at all? -- Joseph S. Myers jos...@codesourcery.com
RE: [PATCH] Fix for PR c/57563
> -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Joseph S. Myers > Sent: Monday, June 10, 2013 11:16 AM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek; mpola...@gcc.gnu.org > Subject: RE: [PATCH] Fix for PR c/57563 > > On Mon, 10 Jun 2013, Iyer, Balaji V wrote: > > > > You don't say what the actual error was, and neither does the original PR. > > > But if it was an ICE from an EXCESS_PRECISION_EXPR getting to the > > > gimplifier, that suggests that c_fully_fold isn't getting called > > > somewhere it should be - and probably calling c_fully_fold is the > > > correct fix rather than inserting a cast. If you can get such ICEs > > > for EXCESS_PRECISION_EXPR, it's quite possible you might get them > > > for C_MAYBE_CONST_EXPR as well (e.g. try using 0 / 0, or compound > > > literals of variably modified type, in various places in the affected > expressions), which should be fixed by using c_fully_fold but not by > inserting a > cast. > > > > It was not. It was actually a type mismatch between double and long > > double caught in verify_gimple_in_seq function. So, is it OK for trunk? > > A cast still doesn't make sense conceptually. Could you give a more detailed > analysis of what the trees look like at this point where you are inserting > this cast, > and how you get to a mismatch? > > EXCESS_PRECISION_EXPR can be thought of as a conversion operator. It should > only appear at the top level of an expression. At the point where excess > precision should be removed - the value converted to its semantic type - > either > the expression with excess precision should be folded using c_fully_fold (if > this is > the expression of an expression statement, or otherwise will go inside a tree > that c_fully_fold does not recurse inside), or the operand of the > EXCESS_PRECISION_EXPR should be converted to the semantic type with the > "convert" function. In neither case is generating a cast appropriate; that's > for > when the user actually wrote a cast in their source code. I looked into it a bit more detail. It was an error on my side. I was removing the excess precision expr layer instead of fully folding it. I did that change (i.e. fully fold the expression) and all the errors seem to go away. Here is the fixed patch that fixes PR c/57563. It passes for 32 bit and 64 bit tests. Here are the changelog entries: gcc/c/ChangeLog 2013-06-10 Balaji V. Iyer * c-array-notation.c (fix_builtin_array_notation_fn): Fully folded excessive precision expressions in function parameters. gcc/testsuite/ChangeLog 2013-06-10 Balaji V. Iyer PR c/57563 * c-c++-common/cilk-plus/AN/builtin_fn_mutating.c (main): Fixed a bug in how we check __sec_reduce_mutating function's result. Thanks, Balaji V. Iyer. > > -- > Joseph S. Myers > jos...@codesourcery.com diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c index b1040da..9298ae0 100644 --- a/gcc/c/c-array-notation.c +++ b/gcc/c/c-array-notation.c @@ -158,10 +158,13 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) func_parm = CALL_EXPR_ARG (an_builtin_fn, 0); while (TREE_CODE (func_parm) == CONVERT_EXPR -|| TREE_CODE (func_parm) == EXCESS_PRECISION_EXPR || TREE_CODE (func_parm) == NOP_EXPR) func_parm = TREE_OPERAND (func_parm, 0); + /* Fully fold any EXCESSIVE_PRECISION EXPR that can occur in the function + parameter. */ + func_parm = c_fully_fold (func_parm, false, NULL); + location = EXPR_LOCATION (an_builtin_fn); if (!find_rank (location, an_builtin_fn, an_builtin_fn, true, &rank)) diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c index 6635565..7c194c2 100644 --- a/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c +++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/builtin_fn_mutating.c @@ -44,11 +44,11 @@ int main(void) max_value = array3[0] * array4[0]; for (ii = 0; ii < 10; ii++) if (array3[ii] * array4[ii] > max_value) { - max_value = array3[ii] * array4[ii]; max_index = ii; } - + for (ii = 0; ii < 10; ii++) +my_func (&max_value, array3[ii] * array4[ii]); #if HAVE_IO for (ii = 0; ii < 10; ii++)
RE: [PATCH] Fix for PR c/57563
On Mon, 10 Jun 2013, Iyer, Balaji V wrote: > > You don't say what the actual error was, and neither does the original PR. > > But if it was an ICE from an EXCESS_PRECISION_EXPR getting to the > > gimplifier, > > that suggests that c_fully_fold isn't getting called somewhere it should be > > - and > > probably calling c_fully_fold is the correct fix rather than inserting a > > cast. If you > > can get such ICEs for EXCESS_PRECISION_EXPR, it's quite possible you might > > get > > them for C_MAYBE_CONST_EXPR as well (e.g. try using 0 / 0, or compound > > literals of variably modified type, in various places in the affected > > expressions), > > which should be fixed by using c_fully_fold but not by inserting a cast. > > It was not. It was actually a type mismatch between double and long > double caught in verify_gimple_in_seq function. So, is it OK for trunk? A cast still doesn't make sense conceptually. Could you give a more detailed analysis of what the trees look like at this point where you are inserting this cast, and how you get to a mismatch? EXCESS_PRECISION_EXPR can be thought of as a conversion operator. It should only appear at the top level of an expression. At the point where excess precision should be removed - the value converted to its semantic type - either the expression with excess precision should be folded using c_fully_fold (if this is the expression of an expression statement, or otherwise will go inside a tree that c_fully_fold does not recurse inside), or the operand of the EXCESS_PRECISION_EXPR should be converted to the semantic type with the "convert" function. In neither case is generating a cast appropriate; that's for when the user actually wrote a cast in their source code. -- Joseph S. Myers jos...@codesourcery.com
RE: [PATCH] Fix for PR c/57563
> -Original Message- > From: Joseph Myers [mailto:jos...@codesourcery.com] > Sent: Monday, June 10, 2013 10:40 AM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek; mpola...@gcc.gnu.org > Subject: Re: [PATCH] Fix for PR c/57563 > > On Sun, 9 Jun 2013, Iyer, Balaji V wrote: > > > Attached, please find a patch that will fix the bug reported in PR > > 57563. There are a couple issues that went wrong. First, in the test > > case, we have a double multiplied to a double. When -std=c99 flag is > > used, they get converted to long double. The way to fix this is to add > > a type cast to the array notation to the same type as identity > > variable and thus they will all be double. > > You don't say what the actual error was, and neither does the original PR. > But if it was an ICE from an EXCESS_PRECISION_EXPR getting to the gimplifier, > that suggests that c_fully_fold isn't getting called somewhere it should be - > and > probably calling c_fully_fold is the correct fix rather than inserting a > cast. If you > can get such ICEs for EXCESS_PRECISION_EXPR, it's quite possible you might get > them for C_MAYBE_CONST_EXPR as well (e.g. try using 0 / 0, or compound > literals of variably modified type, in various places in the affected > expressions), > which should be fixed by using c_fully_fold but not by inserting a cast. It was not. It was actually a type mismatch between double and long double caught in verify_gimple_in_seq function. So, is it OK for trunk? Thanks, Balaji V. Iyer. > > -- > Joseph S. Myers > jos...@codesourcery.com
Re: [PATCH] Fix for PR c/57563
On Sun, 9 Jun 2013, Iyer, Balaji V wrote: > Attached, please find a patch that will fix the bug reported in PR > 57563. There are a couple issues that went wrong. First, in the test > case, we have a double multiplied to a double. When -std=c99 flag is > used, they get converted to long double. The way to fix this is to add a > type cast to the array notation to the same type as identity variable > and thus they will all be double. You don't say what the actual error was, and neither does the original PR. But if it was an ICE from an EXCESS_PRECISION_EXPR getting to the gimplifier, that suggests that c_fully_fold isn't getting called somewhere it should be - and probably calling c_fully_fold is the correct fix rather than inserting a cast. If you can get such ICEs for EXCESS_PRECISION_EXPR, it's quite possible you might get them for C_MAYBE_CONST_EXPR as well (e.g. try using 0 / 0, or compound literals of variably modified type, in various places in the affected expressions), which should be fixed by using c_fully_fold but not by inserting a cast. -- Joseph S. Myers jos...@codesourcery.com
[PATCH] Fix for PR c/57563
Hello Everyone, Attached, please find a patch that will fix the bug reported in PR 57563. There are a couple issues that went wrong. First, in the test case, we have a double multiplied to a double. When -std=c99 flag is used, they get converted to long double. The way to fix this is to add a type cast to the array notation to the same type as identity variable and thus they will all be double. The second issue, was that a sec_reduce_mutating function takes in the address of a "mutating variable" (i.e. the variable that will hold the result), the array notation and a function pointer. For example, for the following code: int a[10], x = 0; void function_name (int *p, int r); __sec_reduce_mutating (&x, a[0:10], function_name); __sec_reduce_mutating should be converted to: for (ii =0; ii < 10; ii++) function_name (&x, a[ii]); In the test case I was not representing this correctly (as shown in the conversion above), but just computing the value that the function should do, thus making the test flaky. I made this fix in the test case. The other advantage of this change is that, in future I can change the what the function does (maybe with #defines and have multiple checks for different function body) and I don't have to change a lot of things. I tried the patch on x86 and x86_64 and it works fine. I am assuming -m32 on x86_64 should have the same behavior as x86. So, is this OK for trunk? Here are the Changelog entries: gcc/c/ChangeLog 2013-06-08 Balaji V. Iyer * c-array-notation.c (fix_builtin_array_notation_fn): Added a cast for all the usage of function parameter to match the identity var. gcc/testsuite/ChangeLog 2013-06-08 Balaji V. Iyer PR c/57563 * c-c++-common/cilk-plus/AN/builtin_fn_mutating.c (main): Fixed a bug in how we check __sec_reduce_mutating function's result. Thanks, Balaji V. Iyer. diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog index 5fbb31f..caf2146 100644 Binary files a/gcc/c/ChangeLog and b/gcc/c/ChangeLog differ diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c index b1040da..1914a24 100644 --- a/gcc/c/c-array-notation.c +++ b/gcc/c/c-array-notation.c @@ -133,7 +133,7 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) bool **count_down, **array_vector; location_t location = UNKNOWN_LOCATION; tree loop_with_init = alloc_stmt_list (); - + tree new_comp_expr = NULL_TREE, identity_expr = NULL_TREE; enum built_in_function an_type = is_cilkplus_reduce_builtin (CALL_EXPR_FN (an_builtin_fn)); if (an_type == BUILT_IN_NONE) @@ -483,10 +483,12 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) new_yes_expr = build_modify_expr (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR, location, func_parm, TREE_TYPE (*new_var)); - new_expr = build_conditional_expr - (location, -build2 (LT_EXPR, TREE_TYPE (*new_var), *new_var, func_parm), false, -new_yes_expr, TREE_TYPE (*new_var), new_no_expr, TREE_TYPE (*new_var)); + new_comp_expr = build2 (LT_EXPR, TREE_TYPE (*new_var), *new_var, + build_c_cast (location, TREE_TYPE (*new_var), + func_parm)); + new_expr = build_conditional_expr (location, new_comp_expr, false, +new_yes_expr, TREE_TYPE (*new_var), +new_no_expr, TREE_TYPE (*new_var)); break; case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN: if (TYPE_MAX_VALUE (new_var_type)) @@ -503,10 +505,12 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) new_yes_expr = build_modify_expr (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR, location, func_parm, TREE_TYPE (*new_var)); - new_expr = build_conditional_expr - (location, -build2 (GT_EXPR, TREE_TYPE (*new_var), *new_var, func_parm), false, -new_yes_expr, TREE_TYPE (*new_var), new_no_expr, TREE_TYPE (*new_var)); + new_comp_expr = build2 (GT_EXPR, TREE_TYPE (*new_var), *new_var, + build_c_cast (location, TREE_TYPE (*new_var), + func_parm)); + new_expr = build_conditional_expr (location, new_comp_expr, false, +new_yes_expr, TREE_TYPE (*new_var), +new_no_expr, TREE_TYPE (*new_var)); break; case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX_IND: new_var_init = build_modify_expr @@ -551,12 +555,13 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) append_to_statement_list (new_no_ind, &new_no_list); append_to_statement_list (new_no_expr, &new_no_list); + new_comp_expr = + build2 (LE_EXPR, TREE_TYPE (array_ind_value), array_ind_value, + build_c_cast (location, TREE_TYPE (array_ind_va