Patchwork Fix for PR c/57490

login
register
mail settings
Submitter Iyer, Balaji V
Date Aug. 12, 2013, 5:16 p.m.
Message ID <BF230D13CA30DD48930C31D4099330003A458A6A@FMSMSX101.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/266587/
State New
Headers show

Comments

Iyer, Balaji V - Aug. 12, 2013, 5:16 p.m.
> -----Original Message-----
> From: Rainer Orth [mailto:ro@CeBiTec.Uni-Bielefeld.DE]
> Sent: Friday, August 09, 2013 7:54 AM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Marek Polacek
> (polacek@redhat.com)
> Subject: Re: [PATCH] Fix for PR c/57490
> 
> Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
> 
> > Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
> >
> >> "Iyer, Balaji V" <balaji.v.iyer@intel.com> writes:
> >>
> >>>> -----Original Message-----
> >>>> From: Jakub Jelinek [mailto:jakub@redhat.com]
> >>>> Sent: Monday, July 01, 2013 1:09 PM
> >>>> To: Iyer, Balaji V
> >>>> Cc: gcc-patches@gcc.gnu.org; Rainer Orth
> >>>> Subject: Re: [PATCH] Fix for PR c/57490
> >>>>
> >>>> On Mon, Jul 01, 2013 at 05:02:57PM +0000, Iyer, Balaji V wrote:
> >>>> > OK. The fixed patch is attached. Here are the ChangeLog entries:
> >>>> >
> >>>> > gcc/cp/ChangeLog
> >>>> > 2013-07-01  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >>>> >
> >>>>
> >>>> Still
> >>>> 	PR c/57490
> >>>> hasn't been added to cp/ChangeLog and c/ChangeLog entries.
> >>>> > --- /dev/null
> >>>> > +++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57490.c
> >>>> > @@ -0,0 +1,25 @@
> >>>>
> >>>
> >>> Fixed as you suggested. Here is the fixed Changelogs and patch is attached.
> >>>
> >>> gcc/cp/ChangeLog
> >>> 2013-07-01  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >>>
> >>>         PR c/57490
> >>>         * cp-array-notation.c (cp_expand_cond_array_notations): Added a
> >>>         check for truth values.
> >>>         (expand_array_notation_exprs): Added truth values case.  Removed an
> >>>         unwanted else.  Added for-loop to walk through subtrees in default
> >>>         case.
> >>>
> >>> gcc/c/ChangeLog
> >>> 2013-07-01  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >>>
> >>>         PR c/57490
> >>>         * c-array-notation.c (fix_conditional_array_notations_1): Added a
> >>>         check for truth values.
> >>>         (expand_array_notation_exprs): Added truth values case.  Removed an
> >>>         unwanted else.  Added for-loop to walk through subtrees in default
> >>>         case.
> >>>
> >>>
> >>> gcc/testsuite/ChangeLog
> >>> 2013-07-01  Balaji V. Iyer  <balaji.v.iyer@intel.com>
> >>>
> >>>         PR c/57490
> >>>         * c-c++-common/cilk-plus/AN/pr57490.c: New test.
> >>
> >> I've just tested this patch on i386-pc-solaris2.10:
> >>
> >> The c-c++-common/cilk-plus/AN/an-if.c test still FAILs for C++:
> >>
> >> FAIL: c-c++-common/cilk-plus/AN/an-if.c  -fcilkplus (internal
> >> compiler error)
> >> FAIL: c-c++-common/cilk-plus/AN/an-if.c  -fcilkplus (test for excess
> >> errors)
> [...]
> > This is still unfixed almost three weeks later.  Balaji, could you
> > please have a look?
> 
> This bug is now unfixed for two months, and no reaction whatsoever on the
> report.  This is getting annoying since it generates large amount of testsuite
> noise.
> 
> Please fix ASAP!
> 

Hi Rainer,

   First off, my sincerest apologies for letting this bug slip the cracks. I am attaching a patch that seem to work fine with the .i file that you have submitted in bugzilla for both C and C++. Please let me know if this fix works for you and if it is OK for trunk.

Here are the Changelog entries:
gcc/c/ChangeLog
2013-08-12  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        PR c/57490
        * c-array-notation.c (fix_conditional_array_notations_1): Added a
        check for truth values.
        (expand_array_notation_exprs): Added truth values case.  Removed an
        unwanted else.  Added for-loop to walk through subtrees in default
        case.

gcc/cp/ChangeLog
2013-08-12  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        PR c/57490
        * cp-array-notation.c (cp_expand_cond_array_notations): Added a
        check for truth values.
        (expand_array_notation_exprs): Added truth values case.  Removed an
        unwanted else.  Added for-loop to walk through subtrees in default
        case.
        * typeck.c (cp_build_binary_op): Inherited the type of the array
        notation for built-in array notation functions.

gcc/testsuite/ChangeLog
2013-07-01  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        PR c/57490
        * c-c++-common/cilk-plus/AN/pr57490.c: New test.


Yours sincerely,

Balaji V. Iyer.


> 	Rainer
> 
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
Rainer Orth - Aug. 13, 2013, 11:18 a.m.
Hi Iyer,

>    First off, my sincerest apologies for letting this bug slip the cracks. I am attaching a patch that seem to work fine with the .i file that you have submitted in bugzilla for both C and C++. Please let me know if this fix works for you and if it is OK for trunk.

thanks for the patch.  I've just bootstrapped it on i386-pc-solaris2.10
and all an-if.c failures are gone.  That said, I wonder why we need the
separate pr57490.c testcase, which is practically a preprocessed version
of an-if.c with the HAVE_IO code removed.

I cannot approve the patch, though.

	Rainer
Iyer, Balaji V - Aug. 13, 2013, 1:18 p.m.
> -----Original Message-----
> From: Rainer Orth [mailto:ro@CeBiTec.Uni-Bielefeld.DE]
> Sent: Tuesday, August 13, 2013 7:18 AM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Marek Polacek
> (polacek@redhat.com)
> Subject: Re: [PATCH] Fix for PR c/57490
> 
> Hi Iyer,
> 
> >    First off, my sincerest apologies for letting this bug slip the cracks. I am
> attaching a patch that seem to work fine with the .i file that you have submitted
> in bugzilla for both C and C++. Please let me know if this fix works for you and if
> it is OK for trunk.
> 
> thanks for the patch.  I've just bootstrapped it on i386-pc-solaris2.10 and all an-
> if.c failures are gone.  That said, I wonder why we need the separate pr57490.c
> testcase, which is practically a preprocessed version of an-if.c with the HAVE_IO
> code removed.

Well, it has assert in it. A while back someone (I think it was Jason Merrill, but I could be wrong) told me to replace all asserts with builtin_abort. But the content inside the file is the same.

> 
> I cannot approve the patch, though.
> 

OK. Can I ask why? Is it because you do not have an authority or is it because you see some errors (e.g. formatting errors) in it? If it is the latter, can you tell me what the errors are so that I can go and fix them?


> 	Rainer
> 
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
Rainer Orth - Aug. 13, 2013, 1:37 p.m.
Hi Iyer,

>> thanks for the patch.  I've just bootstrapped it on i386-pc-solaris2.10
>> and all an-
>> if.c failures are gone.  That said, I wonder why we need the separate
>> pr57490.c
>> testcase, which is practically a preprocessed version of an-if.c with the
>> HAVE_IO
>> code removed.
>
> Well, it has assert in it. A while back someone (I think it was Jason
> Merrill, but I could be wrong) told me to replace all asserts with
> builtin_abort. But the content inside the file is the same.

in that case I'd suggest modifying an-if.c accordingly and not adding
pr57490.c.

>> I cannot approve the patch, though.
>
> OK. Can I ask why? Is it because you do not have an authority or is it
> because you see some errors (e.g. formatting errors) in it? If it is the
> latter, can you tell me what the errors are so that I can go and fix them?

It's simply not my area of authority.

	Rainer
Iyer, Balaji V - Aug. 13, 2013, 1:40 p.m.
> -----Original Message-----
> From: Rainer Orth [mailto:ro@CeBiTec.Uni-Bielefeld.DE]
> Sent: Tuesday, August 13, 2013 9:38 AM
> To: Iyer, Balaji V
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Marek Polacek
> (polacek@redhat.com)
> Subject: Re: [PATCH] Fix for PR c/57490
> 
> Hi Iyer,
> 
> >> thanks for the patch.  I've just bootstrapped it on
> >> i386-pc-solaris2.10 and all an- if.c failures are gone.  That said, I
> >> wonder why we need the separate pr57490.c testcase, which is
> >> practically a preprocessed version of an-if.c with the HAVE_IO code
> >> removed.
> >
> > Well, it has assert in it. A while back someone (I think it was Jason
> > Merrill, but I could be wrong) told me to replace all asserts with
> > builtin_abort. But the content inside the file is the same.
> 
> in that case I'd suggest modifying an-if.c accordingly and not adding pr57490.c.
> 

Well, on x86, an-if.c works fine without this modifications. It is just that in sparc the code is broken up a bit differently.

> >> I cannot approve the patch, though.
> >
> > OK. Can I ask why? Is it because you do not have an authority or is it
> > because you see some errors (e.g. formatting errors) in it? If it is
> > the latter, can you tell me what the errors are so that I can go and fix them?
> 
> It's simply not my area of authority.
> 

Ok. Thanks for looking into this quickly though.

Sincerely,

Balaji V. Iyer.

> 	Rainer
> 
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
Jason Merrill - Aug. 16, 2013, 6:07 p.m.
On 08/12/2013 01:16 PM, Iyer, Balaji V wrote:
> +  /* If it is a built-in array notation function, then the return type of
> +     the function is the type of the array passed in as array notation.  */

How can the function return an array?

Jason
Iyer, Balaji V - Aug. 16, 2013, 6:13 p.m.
> -----Original Message-----
> From: Jason Merrill [mailto:jason@redhat.com]
> Sent: Friday, August 16, 2013 2:08 PM
> To: Iyer, Balaji V; Rainer Orth
> Cc: Jakub Jelinek; gcc-patches@gcc.gnu.org; Marek Polacek
> (polacek@redhat.com)
> Subject: Re: [PATCH] Fix for PR c/57490
> 
> On 08/12/2013 01:16 PM, Iyer, Balaji V wrote:
> > +  /* If it is a built-in array notation function, then the return type of
> > +     the function is the type of the array passed in as array notation.  */
> 
> How can the function return an array?
> 

I guess I worded the comment poorly. Well, what I was going to say is this:

float x,  A[10];
x = __sec_reduce_add (A[:]); // The sec_reduce_add function's return type is the type of A[] which is float.

int y, B[10];
y = __sec_reduce_add (B[:]); // ...the return type of __sec_reduce_add function is same as B[] which is int


Thanks,

Balaji V. Iyer.

> Jason
Jason Merrill - Aug. 17, 2013, 4:55 a.m.
On 08/16/2013 02:13 PM, Iyer, Balaji V wrote:
>>> +  /* If it is a built-in array notation function, then the return type of
>>> +     the function is the type of the array passed in as array notation.  */
>>
>> How can the function return an array?
>
> float x,  A[10];
> x = __sec_reduce_add (A[:]); // The sec_reduce_add function's return type is the type of A[] which is float.

Ah, then the comment should say "...is the element type of the array...".

Jason

Patch

diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 7788f7b..5747bcb 100644
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -906,6 +906,8 @@  fix_conditional_array_notations_1 (tree stmt)
     cond = COND_EXPR_COND (stmt);
   else if (TREE_CODE (stmt) == SWITCH_EXPR)
     cond = SWITCH_COND (stmt);
+  else if (truth_value_p (TREE_CODE (stmt)))
+    cond = TREE_OPERAND (stmt, 0);
   else
     /* Otherwise dont even touch the statement.  */
     return stmt;
@@ -1232,6 +1234,12 @@  expand_array_notation_exprs (tree t)
     case BIND_EXPR:
       t = expand_array_notation_exprs (BIND_EXPR_BODY (t));
       return t;
+    case TRUTH_ORIF_EXPR:
+    case TRUTH_ANDIF_EXPR:
+    case TRUTH_OR_EXPR:
+    case TRUTH_AND_EXPR:
+    case TRUTH_XOR_EXPR:
+    case TRUTH_NOT_EXPR:
     case COND_EXPR:
       t = fix_conditional_array_notations (t);
 
@@ -1246,8 +1254,6 @@  expand_array_notation_exprs (tree t)
 	    COND_EXPR_ELSE (t) =
 	      expand_array_notation_exprs (COND_EXPR_ELSE (t));
 	}
-      else
-	t = expand_array_notation_exprs (t);
       return t;
     case STATEMENT_LIST:
       {
@@ -1284,6 +1290,10 @@  expand_array_notation_exprs (tree t)
 	 Replace those with just void zero node.  */
       t = void_zero_node;
     default:
+      for (int ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (t)); ii++)
+	if (contains_array_notation_expr (TREE_OPERAND (t, ii)))
+	  TREE_OPERAND (t, ii) =
+	    expand_array_notation_exprs (TREE_OPERAND (t, ii));
       return t;
     }
   return t;
diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
index eb6a70d..f4581f0 100644
--- a/gcc/cp/cp-array-notation.c
+++ b/gcc/cp/cp-array-notation.c
@@ -857,6 +857,19 @@  cp_expand_cond_array_notations (tree orig_stmt)
 	  return error_mark_node;
 	}
     }
+  else if (truth_value_p (TREE_CODE (orig_stmt)))
+    {
+      size_t left_rank = 0, right_rank = 0;
+      tree left_expr = TREE_OPERAND (orig_stmt, 0);
+      tree right_expr = TREE_OPERAND (orig_stmt, 1);
+      if (!find_rank (EXPR_LOCATION (left_expr), left_expr, left_expr, true,
+		      &left_rank)
+	  || !find_rank (EXPR_LOCATION (right_expr), right_expr, right_expr,
+			 true, &right_rank))
+	return error_mark_node;
+      if (right_rank == 0 && left_rank == 0)
+	return orig_stmt;
+    }
 
   if (!find_rank (EXPR_LOCATION (orig_stmt), orig_stmt, orig_stmt, true,
 		  &rank))
@@ -1213,6 +1226,12 @@  expand_array_notation_exprs (tree t)
       if (TREE_OPERAND (t, 0) == error_mark_node)
 	return TREE_OPERAND (t, 0); 
       return t;
+    case TRUTH_ANDIF_EXPR:
+    case TRUTH_ORIF_EXPR:
+    case TRUTH_AND_EXPR:
+    case TRUTH_OR_EXPR:
+    case TRUTH_XOR_EXPR:
+    case TRUTH_NOT_EXPR:
     case COND_EXPR:
       t = cp_expand_cond_array_notations (t);
       if (TREE_CODE (t) == COND_EXPR)
@@ -1222,8 +1241,6 @@  expand_array_notation_exprs (tree t)
 	  COND_EXPR_ELSE (t) =
 	    expand_array_notation_exprs (COND_EXPR_ELSE (t));
 	}
-      else
-	t = expand_array_notation_exprs (t);
       return t;
     case FOR_STMT:
       if (contains_array_notation_expr (FOR_COND (t)))
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index e09c325..ba5286f 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3954,6 +3954,20 @@  cp_build_binary_op (location_t location,
   type0 = TREE_TYPE (op0); 
   type1 = TREE_TYPE (op1);
 
+  /* If it is a built-in array notation function, then the return type of
+     the function is the type of the array passed in as array notation.  */
+  if (flag_enable_cilkplus && TREE_CODE (op1) == CALL_EXPR
+      && is_cilkplus_reduce_builtin (CALL_EXPR_FN (op1)))
+    {
+      tree array_ntn = CALL_EXPR_ARG (op1, 0);
+      type1 = TREE_TYPE (array_ntn);
+    }
+  if (flag_enable_cilkplus && TREE_CODE (op0) == CALL_EXPR
+      && is_cilkplus_reduce_builtin (CALL_EXPR_FN (op0)))
+    {
+      tree array_ntn = CALL_EXPR_ARG (op0, 0);
+      type0 = TREE_TYPE (array_ntn);
+    }
   /* The expression codes of the data types of the arguments tell us
      whether the arguments are integers, floating, pointers, etc.  */
   code0 = TREE_CODE (type0);
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57490.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57490.c
new file mode 100644
index 0000000..db38b30
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57490.c
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+/* { dg-options "-fcilkplus" } */
+
+const int n = 8;
+float x[8], y[8], z[8];
+int main() {
+    int i = 0;
+    float x_sum =0;
+    for(i=1; i<=5; i+=4 ) {
+        x[0:n] = 3;
+        y[0:n] = i;
+        z[0:n] = 0;
+        (void)((__sec_reduce_add(x[0:n])==3*n) || (__builtin_abort (), 0));
+        (void)((__sec_reduce_add(y[0:n])==i*n) || (__builtin_abort (), 0));
+        (void)((__sec_reduce_add(z[0:n])==0) || (__builtin_abort (), 0));
+
+        if (x[0:n] >= y[0:n]) {
+            z[0:n] = x[0:n] - y[0:n];
+        } else {
+            z[0:n] = x[0:n] + y[0:n];
+        }
+        (void)((__sec_reduce_add(x[0:n])==3*n) || (__builtin_abort (), 0));
+        (void)((__sec_reduce_add(y[0:n])==i*n) || (__builtin_abort (), 0));
+        (void)((__sec_reduce_add(z[0:n])==(3>=i?3-i:3+i)*n) 
+	       || (__builtin_abort (), 0));
+    }
+    return 0;
+}