Message ID | BF230D13CA30DD48930C31D4099330003A42D3CA@FMSMSX101.amr.corp.intel.com |
---|---|
State | New |
Headers | show |
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.
> -----Original Message----- > From: Joseph Myers [mailto:joseph@codesourcery.com] > Sent: Monday, June 10, 2013 10:40 AM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek; mpolacek@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 > joseph@codesourcery.com
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.
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_value), + func_parm)); new_expr = build_conditional_expr - (location, - build2 (LE_EXPR, TREE_TYPE (array_ind_value), array_ind_value, - func_parm), - false, - new_yes_list, TREE_TYPE (*new_var), new_no_list, TREE_TYPE (*new_var)); + (location, new_comp_expr, false, new_yes_list, TREE_TYPE (*new_var), + new_no_list, TREE_TYPE (*new_var)); break; case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN_IND: new_var_init = build_modify_expr @@ -601,24 +606,34 @@ 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 (GE_EXPR, TREE_TYPE (array_ind_value), array_ind_value, + build_c_cast (location, TREE_TYPE (array_ind_value), + func_parm)); new_expr = build_conditional_expr - (location, - build2 (GE_EXPR, TREE_TYPE (array_ind_value), array_ind_value, - func_parm), - false, - new_yes_list, TREE_TYPE (*new_var), new_no_list, TREE_TYPE (*new_var)); + (location, new_comp_expr, false, new_yes_list, TREE_TYPE (*new_var), + new_no_list, TREE_TYPE (*new_var)); break; case BUILT_IN_CILKPLUS_SEC_REDUCE: new_var_init = build_modify_expr (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR, location, identity_value, new_var_type); - new_call_expr = build_call_expr (call_fn, 2, *new_var, func_parm); + new_call_expr = build_call_expr (call_fn, 2, *new_var, + build_c_cast (location, new_var_type, + func_parm)); new_expr = build_modify_expr (location, *new_var, TREE_TYPE (*new_var), NOP_EXPR, location, new_call_expr, TREE_TYPE (*new_var)); break; case BUILT_IN_CILKPLUS_SEC_REDUCE_MUTATING: - new_expr = build_call_expr (call_fn, 2, identity_value, func_parm); + if (TREE_CODE (TREE_TYPE (identity_value)) == POINTER_TYPE) + new_var_type = TREE_TYPE (TREE_TYPE (identity_value)); + identity_expr = TREE_OPERAND (identity_value, 0); + identity_value = build_fold_addr_expr (identity_expr); + TREE_USED (identity_expr) = 1; + new_expr = build_call_expr (call_fn, 2, identity_value, + build_c_cast (location, new_var_type, + func_parm)); break; default: gcc_unreachable (); diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index b8fe362..6c6c50c 100644 Binary files a/gcc/testsuite/ChangeLog and b/gcc/testsuite/ChangeLog differ 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++)