Patchwork Expanding array notations inside conditions

login
register
mail settings
Submitter Iyer, Balaji V
Date May 31, 2013, 4 a.m.
Message ID <BF230D13CA30DD48930C31D4099330003A429BFB@FMSMSX101.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/247840/
State New
Headers show

Comments

Iyer, Balaji V - May 31, 2013, 4 a.m.
Hello Everyone,
	When I was looking at the erroneous test on PR 57452, I found out that array notations in conditions were not expanded correctly. The rank for the array notation condition must be same (or equal to zero) as the rank of the array notations inside the else-block and then-block (or they could be zero). I found out that GCC was not detecting these errors. I am very sorry for this mishap. The attached patch should fix that issue. I have also enclosed a test-suite code to make sure this gets caught in future. 
	
	It is tested on x86_64 and seem to work OK. The only test that it is failing is the erronous test called if_test.c, and if patch specified in (http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01815.html) is applied, then that test will pass. 

Is this OK for trunk?

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

        * c-typeck.c (c_finish_if_stmt): Added a check to see if the rank of the
        condition of the if-statement matches the rank of else-block and then-
        block when array notations are used.
        * c-parser.c (c_parser_declaration_or_fndef): Expanded array notation
        expression after the entire function body is parsed.
        (c_parser_expr_no_commas): Delayed creating array notation expressions
        to the end of function parsing.
        * c-array-notation.c (fix_conditional_array_notations_1): Expanded the
        whole if-statement instead of just the condition.
        (expand_array_notation_exprs): Added MODIFY_EXPR case.

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

        * c-c++-common/cilk-plus/AN/if_test_errors.c (main): New testcase.
        * c-c++-common/cilk-plus/AN/rank_mismatch.c: Added a '-w' option to
        dg-option and an header comment.


Thanks,

Balaji V. Iyer.
Jeff Law - June 3, 2013, 8:32 p.m.
On 05/30/2013 10:00 PM, Iyer, Balaji V wrote:
> Hello Everyone,
> 	When I was looking at the erroneous test on PR 57452, I found out that array notations in conditions were not expanded correctly. The rank for the array notation condition must be same (or equal to zero) as the rank of the array notations inside the else-block and then-block (or they could be zero). I found out that GCC was not detecting these errors. I am very sorry for this mishap. The attached patch should fix that issue. I have also enclosed a test-suite code to make sure this gets caught in future.
> 	
> 	It is tested on x86_64 and seem to work OK. The only test that it is failing is the erronous test called if_test.c, and if patch specified in (http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01815.html) is applied, then that test will pass.
>
> Is this OK for trunk?
>
> Here are the ChangeLog entries:
> gcc/c/ChangeLog
> 2013-05-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>
>          * c-typeck.c (c_finish_if_stmt): Added a check to see if the rank of the
>          condition of the if-statement matches the rank of else-block and then-
>          block when array notations are used.
>          * c-parser.c (c_parser_declaration_or_fndef): Expanded array notation
>          expression after the entire function body is parsed.
>          (c_parser_expr_no_commas): Delayed creating array notation expressions
>          to the end of function parsing.
>          * c-array-notation.c (fix_conditional_array_notations_1): Expanded the
>          whole if-statement instead of just the condition.
>          (expand_array_notation_exprs): Added MODIFY_EXPR case.
>
> gcc/testsuite/ChangeLog
> 2013-05-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>
>          * c-c++-common/cilk-plus/AN/if_test_errors.c (main): New testcase.
>          * c-c++-common/cilk-plus/AN/rank_mismatch.c: Added a '-w' option to
>          dg-option and an header comment.
OK after fixing the new tests not to rely on argc/argv.

jeff

Patch

diff --git gcc/c/ChangeLog gcc/c/ChangeLog
old mode 100644
new mode 100755
index d33fa2b..828d7c8
--- gcc/c/ChangeLog
+++ gcc/c/ChangeLog
@@ -1,3 +1,16 @@ 
+2013-05-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+	* c-typeck.c (c_finish_if_stmt): Added a check to see if the rank of the
+	condition of the if-statement matches the rank of else_block and then-
+	block when array notations are used.
+	* c-parser.c (c_parser_declaration_or_fndef): Expanded array notation
+	expression after the entire function body is parsed.
+	(c_parser_expr_no_commas): Delayed creating array notation expressions
+	to the end of function parsing.
+	* c-array-notation.c (fix_conditional_array_notations_1): Expanded the
+	whole if-statement instead of just the condition.
+	(expand_array_notation_exprs): Added MODIFY_EXPR case.
+
 2013-05-29  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	PR bootstrap/57450
diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
old mode 100644
new mode 100755
index bf139a8..681e111
--- gcc/c/c-array-notation.c
+++ gcc/c/c-array-notation.c
@@ -1875,7 +1875,7 @@  fix_conditional_array_notations_1 (tree stmt)
   if (!find_rank (location, cond, cond, false, &rank))
     return error_mark_node;
   
-  extract_array_notation_exprs (cond, false, &array_list);
+  extract_array_notation_exprs (stmt, false, &array_list);
   loop_init = push_stmt_list ();
   for (ii = 0; ii < vec_safe_length (array_list); ii++)
     { 
@@ -1895,12 +1895,12 @@  fix_conditional_array_notations_1 (tree stmt)
 	      vec_safe_push (sub_list, array_node);
 	      vec_safe_push (new_var_list, new_var);
 	      add_stmt (builtin_loop);
-	      replace_array_notations (&cond, false, sub_list, new_var_list); 
+	      replace_array_notations (&stmt, false, sub_list, new_var_list); 
 	    }
 	}
     }
 
-  if (!find_rank (location, cond, cond, true, &rank))
+  if (!find_rank (location, stmt, stmt, true, &rank))
     {
       pop_stmt_list (loop_init);
       return error_mark_node;
@@ -1911,7 +1911,7 @@  fix_conditional_array_notations_1 (tree stmt)
       pop_stmt_list (loop_init); 
       return loop_init;
     }  
-  extract_array_notation_exprs (cond, true, &array_list);
+  extract_array_notation_exprs (stmt, true, &array_list);
 
   if (vec_safe_length (array_list) == 0)
     return stmt;
@@ -2761,6 +2761,18 @@  expand_array_notation_exprs (tree t)
 	    expand_array_notation_exprs (*tsi_stmt_ptr (ii_tsi));
       }
       return t;
+    case MODIFY_EXPR:
+      {
+	location_t loc = EXPR_HAS_LOCATION (t) ? EXPR_LOCATION (t) :
+	  UNKNOWN_LOCATION;
+	tree lhs = TREE_OPERAND (t, 0);
+	tree rhs = TREE_OPERAND (t, 1);
+	location_t rhs_loc = EXPR_HAS_LOCATION (rhs) ? EXPR_LOCATION (rhs) :
+	  UNKNOWN_LOCATION;
+	t = build_array_notation_expr (loc, lhs, TREE_TYPE (lhs), NOP_EXPR,
+				       rhs_loc, rhs, TREE_TYPE (rhs));
+	return t;
+      }
     case CALL_EXPR:
       t = fix_array_notation_call_expr (t);
       return t;
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
old mode 100644
new mode 100755
index b89d8c1..d6a500e
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -1756,6 +1756,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
       DECL_STRUCT_FUNCTION (current_function_decl)->function_start_locus
 	= c_parser_peek_token (parser)->location;
       fnbody = c_parser_compound_statement (parser);
+      if (flag_enable_cilkplus && contains_array_notation_expr (fnbody))
+	fnbody = expand_array_notation_exprs (fnbody);
       if (nested)
 	{
 	  tree decl = current_function_decl;
@@ -5445,20 +5447,9 @@  c_parser_expr_no_commas (c_parser *parser, struct c_expr *after)
   rhs = c_parser_expr_no_commas (parser, NULL);
   rhs = default_function_array_read_conversion (exp_location, rhs);
   
-  /* The line below is where the statement has the form:
-     A = B, where A and B contain array notation exprs. So this is where
-     we handle those.  */
-  if (flag_enable_cilkplus
-      && (contains_array_notation_expr (lhs.value)
-	  || contains_array_notation_expr (rhs.value)))
-    ret.value = build_array_notation_expr (op_location, lhs.value,
-					   lhs.original_type, code,
-					   exp_location, rhs.value,
-					   rhs.original_type);
-  else
-    ret.value = build_modify_expr (op_location, lhs.value, lhs.original_type,
-				   code, exp_location, rhs.value,
-				   rhs.original_type);
+  ret.value = build_modify_expr (op_location, lhs.value, lhs.original_type,
+				 code, exp_location, rhs.value,
+				 rhs.original_type);
   if (code == NOP_EXPR)
     ret.original_code = MODIFY_EXPR;
   else
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
old mode 100644
new mode 100755
index 749c8e2..e5e1455
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -8983,6 +8983,34 @@  c_finish_if_stmt (location_t if_locus, tree cond, tree then_block,
 {
   tree stmt;
 
+  /* If the condition has array notations, then the rank of the then_block and
+     else_block must be either 0 or be equal to the rank of the condition.  If
+     the condition does not have array notations then break them up as it is
+     broken up in a normal expression.  */
+  if (flag_enable_cilkplus && contains_array_notation_expr (cond))
+    {
+      size_t then_rank = 0, cond_rank = 0, else_rank = 0;
+      if (!find_rank (if_locus, cond, cond, true, &cond_rank))
+	return;
+      if (then_block
+	  && !find_rank (if_locus, then_block, then_block, true, &then_rank))
+	return;
+      if (else_block
+	  && !find_rank (if_locus, else_block, else_block, true, &else_rank)) 
+	return;
+      if (cond_rank != then_rank && then_rank != 0)
+	{
+	  error_at (if_locus, "rank-mismatch between if-statement%'s condition"
+		    " and the then-block");
+	  return;
+	}
+      else if (cond_rank != else_rank && else_rank != 0)
+	{
+	  error_at (if_locus, "rank-mismatch between if-statement%'s condition"
+		    " and the else-block");
+	  return;
+	}
+    }
   /* Diagnose an ambiguous else if if-then-else is nested inside if-then.  */
   if (warn_parentheses && nested_if && else_block == NULL)
     {
diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog
old mode 100644
new mode 100755
index 27bf134..5cba191
--- gcc/testsuite/ChangeLog
+++ gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2013-05-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+	* c-c++-common/cilk-plus/AN/if_test_errors.c (main): New testcase.
+	* c-c++-common/cilk-plus/AN/rank_mismatch.c: Added a '-w' option to
+	dg-option and an header comment.
+
 2013-05-30  Tobias Burnus  <burnus@net-b.de>
 
 	PR middle-end/57073
diff --git gcc/testsuite/c-c++-common/cilk-plus/AN/if_test_errors.c gcc/testsuite/c-c++-common/cilk-plus/AN/if_test_errors.c
new file mode 100755
index 0000000..5e88dce
--- /dev/null
+++ gcc/testsuite/c-c++-common/cilk-plus/AN/if_test_errors.c
@@ -0,0 +1,56 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+#include <stdlib.h>
+int main (int argc, char **argv)
+{
+  int x = 3, y, z, array[10], array2[10], TwodArray[10][10], jj,kk,ll ;
+  int array2_check[10], array2d_check[10][10], array2d[10][10];
+  int FourDArray[10][10][10][10], array4[10][10][10][10];
+  int array4_check[10][10][10][10];
+  int ii = 0; 
+
+  x = atoi (argv[1])-10;
+  y = atoi (argv[1])/2;
+  z = (atoi (argv[1]))/5;
+
+  if (!array[:]) /* This is OK! */
+    array2[:] = 5;
+  else
+    array2[:] = 10;
+  if (!(array[0:10:1] + array[0:10:1])) /* { dg-error "condition and the then-block" } */
+    array2d[:][:] = 5;
+  else
+    array2[:] = 10;
+
+  if (!(array[0:10:1] + array[0:10:1])) /* { dg-error "condition and the else-block" } */
+    array2[:] = 5;
+  else
+    array2d[:][:] = 10;
+
+
+  if (TwodArray[:][:] != 10) /* { dg-error "condition and the then-block" } */
+    array2[:] = 10; 
+  else
+    array2[:] = 5;
+
+  if (FourDArray[43][:][:][:] != 10) /* This is OK!  */ 
+    array4[45][:][:][:] = 10; 
+  else
+    array4[32][:][:][:] = 5;
+
+  /* atoi(argv[1]) == 10, so it will convert all 10's to 5's */
+  if (FourDArray[42][0:10:1][9:10:-1][0:5:2] != 10) /* { dg-error "condition and the then-block" } */
+    array4[0:10:1][0:5:2][9:10:-1][0:5:2] = 10; 
+  else
+    array4[0:10:1][0:5:2][9:10:-1][0:5:2] = 5;
+
+  /* atoi(argv[1]) == 10, so it will convert all 10's to 5's */
+  if (FourDArray[0:10:1][0:5:2][9:10:-1][x:y:z] +
+      FourDArray[0:10:1][0:5:2][9:-10:1][x:y:z]  != 20) 
+    array4[0:10:1][0:5:2][9:10:-1][x:y:z] = 10; 
+  else
+    array4[0:10][0:5:2][9:10:-1][x:y:z] = 5;
+
+  return 0;
+}
diff --git gcc/testsuite/c-c++-common/cilk-plus/AN/rank_mismatch.c gcc/testsuite/c-c++-common/cilk-plus/AN/rank_mismatch.c
index a8c9dab..352d670 100644
--- gcc/testsuite/c-c++-common/cilk-plus/AN/rank_mismatch.c
+++ gcc/testsuite/c-c++-common/cilk-plus/AN/rank_mismatch.c
@@ -1,5 +1,8 @@ 
 /* { dg-do compile } */
-/* { dg-options "-fcilkplus" } */
+/* { dg-options "-fcilkplus -w" } */
+
+/* We use -w because in the first error, there will be a warning of setting an
+   integer to a pointer.  Just ignore it to expose the rank mismatch error.  */
 
 int main (int argc, char **argv)
 {