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++)
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