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
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 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: 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 balaji.v.i...@intel.com * 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 balaji.v.i...@intel.com 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: 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: 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++)
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 balaji.v.i...@intel.com * 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 balaji.v.i...@intel.com 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
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
[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 balaji.v.i...@intel.com * 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 balaji.v.i...@intel.com 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, +