Re: [PATCH] Fix for PR c/57563

2013-06-10 Thread Joseph S. Myers 
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

2013-06-10 Thread Iyer, Balaji V 


 -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

2013-06-10 Thread Joseph S. Myers 
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

2013-06-10 Thread Iyer, Balaji V 


 -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

2013-06-10 Thread Joseph S. Myers 
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

2013-06-10 Thread Iyer, Balaji V 


 -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

2013-06-10 Thread Joseph S. Myers 
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