diff mbox

Fix for PR c/57563

Message ID BF230D13CA30DD48930C31D4099330003A42D776@FMSMSX101.amr.corp.intel.com
State New
Headers show

Commit Message

Iyer, Balaji V June 10, 2013, 5:19 p.m. UTC 
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@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; mpolacek@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.iyer@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.iyer@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
> joseph@codesourcery.com

Comments

Joseph Myers June 10, 2013, 9:18 p.m. UTC | #1 
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?
Iyer, Balaji V June 10, 2013, 10:18 p.m. UTC | #2 
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.iyer@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.iyer@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.
diff mbox

Patch

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