[PR C/79116] ICE with CilkPlus array notation and _Cilk_for (C front-end)

[PR C/79116] ICE with CilkPlus array notation and _Cilk_for (C front-end)

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

This is the same as pr70565 but it fails in an entirely different manner 
in the C front-end.

The problem here is that the parser builds an ARRAY_NOTATION_REF with a 
type of ptrdiff for length and stride.  Later in 
cilkplus_extract_an_triplets we convert convert length and stride to an 
integer_type_node.  This causes create_array_refs() to use a stride of 
integer_type, while the start is still a ptrdiff (verify_gimple ICE, boom).

The attached patch converts `start' to an integer_type to match the 
length and stride.  We could either do this, or do a fold_convert if 
!useless_type_conversion_p in create_array_refs.  I didn't want to look 
into cilkplus too deeply as to why we have different types, because (a) 
I don't care (b) we're probably going to deprecate Cilk Plus, no?

OK?


[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 1413 bytes --]

commit 494d38235e7a250f3f3b4d4c1950be9208917cce
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Jan 17 08:27:57 2017 -0500

            PR c/79116
            * array-notation-common.c (cilkplus_extract_an_triplets): Convert
            start type to integer_type.

diff --git a/gcc/c-family/array-notation-common.c b/gcc/c-family/array-notation-common.c
index 061c203..3b95332 100644
--- a/gcc/c-family/array-notation-common.c
+++ b/gcc/c-family/array-notation-common.c
@@ -628,7 +628,9 @@ cilkplus_extract_an_triplets (vec<tree, va_gc> *list, size_t size, size_t rank,
 	  tree ii_tree = array_exprs[ii][jj];
 	  (*node)[ii][jj].is_vector = true;
 	  (*node)[ii][jj].value = ARRAY_NOTATION_ARRAY (ii_tree);
-	  (*node)[ii][jj].start = ARRAY_NOTATION_START (ii_tree);
+	  (*node)[ii][jj].start
+	    = fold_build1 (CONVERT_EXPR, integer_type_node,
+			   ARRAY_NOTATION_START (ii_tree));
 	  (*node)[ii][jj].length
 	    = fold_build1 (CONVERT_EXPR, integer_type_node,
 			   ARRAY_NOTATION_LENGTH (ii_tree));
diff --git a/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c b/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c
new file mode 100644
index 0000000..9206aaf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+int array[1024];
+void foo()
+{
+  _Cilk_for (int i = 0; i < 512; ++i)
+    array[:] = __sec_implicit_index(0);
+}

Re: [PR C/79116] ICE with CilkPlus array notation and _Cilk_for (C front-end)

[Jakub Jelinek]

On Tue, Jan 17, 2017 at 09:22:52AM -0500, Aldy Hernandez wrote:
> This is the same as pr70565 but it fails in an entirely different manner in
> the C front-end.
> 
> The problem here is that the parser builds an ARRAY_NOTATION_REF with a type
> of ptrdiff for length and stride.  Later in cilkplus_extract_an_triplets we
> convert convert length and stride to an integer_type_node.  This causes
> create_array_refs() to use a stride of integer_type, while the start is
> still a ptrdiff (verify_gimple ICE, boom).
> 
> The attached patch converts `start' to an integer_type to match the length
> and stride.  We could either do this, or do a fold_convert if
> !useless_type_conversion_p in create_array_refs.  I didn't want to look into
> cilkplus too deeply as to why we have different types, because (a) I don't
> care (b) we're probably going to deprecate Cilk Plus, no?

Conceptually, using integer_type_node for these things is complete wrong,
unless the Cilk+ specification says that all the array notation expressions
are converted to int.  Because forcing the int there means that it will
misbehave on very large arrays (over 2GB elements).
So much better would be to have the expressions converted to sizetype or
something similar that the middle-end works with (yes, it is unsigned, so
if it needs to be signed somewhere, we'd need corresponding signed type for
that).

The question is where all is the integer_type_node in the Cilk+ lowering
hardcoded and how hard would it be to fix it up.

If it is too hard, I guess I can live with this patch, but there should be a
PR that it needs to be fixed not to hardcode int type which is inappropriate
for sizes/lengths.

And the more important question is if Intel is willing to maintain Cilk+ in
GCC, or if we should deprecate it (and, if the latter, if already in GCC7
deprecate, remove in GCC8, or deprecate in GCC8, remove in GCC9).
There are various Cilk+ related PRs around on which nothing has been done
for many months.

> commit 494d38235e7a250f3f3b4d4c1950be9208917cce
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Tue Jan 17 08:27:57 2017 -0500
> 
>             PR c/79116
>             * array-notation-common.c (cilkplus_extract_an_triplets): Convert
>             start type to integer_type.
> 
> diff --git a/gcc/c-family/array-notation-common.c b/gcc/c-family/array-notation-common.c
> index 061c203..3b95332 100644
> --- a/gcc/c-family/array-notation-common.c
> +++ b/gcc/c-family/array-notation-common.c
> @@ -628,7 +628,9 @@ cilkplus_extract_an_triplets (vec<tree, va_gc> *list, size_t size, size_t rank,
>  	  tree ii_tree = array_exprs[ii][jj];
>  	  (*node)[ii][jj].is_vector = true;
>  	  (*node)[ii][jj].value = ARRAY_NOTATION_ARRAY (ii_tree);
> -	  (*node)[ii][jj].start = ARRAY_NOTATION_START (ii_tree);
> +	  (*node)[ii][jj].start
> +	    = fold_build1 (CONVERT_EXPR, integer_type_node,
> +			   ARRAY_NOTATION_START (ii_tree));
>  	  (*node)[ii][jj].length
>  	    = fold_build1 (CONVERT_EXPR, integer_type_node,
>  			   ARRAY_NOTATION_LENGTH (ii_tree));
> diff --git a/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c b/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c
> new file mode 100644
> index 0000000..9206aaf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/cilk-plus/pr79116.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fcilkplus" } */
> +
> +int array[1024];
> +void foo()
> +{
> +  _Cilk_for (int i = 0; i < 512; ++i)
> +    array[:] = __sec_implicit_index(0);
> +}


	Jakub

Re: [PR C/79116] ICE with CilkPlus array notation and _Cilk_for (C front-end)

[Aldy Hernandez]

On 01/17/2017 09:41 AM, Jakub Jelinek wrote:
> On Tue, Jan 17, 2017 at 09:22:52AM -0500, Aldy Hernandez wrote:
>> This is the same as pr70565 but it fails in an entirely different manner in
>> the C front-end.
>>
>> The problem here is that the parser builds an ARRAY_NOTATION_REF with a type
>> of ptrdiff for length and stride.  Later in cilkplus_extract_an_triplets we
>> convert convert length and stride to an integer_type_node.  This causes
>> create_array_refs() to use a stride of integer_type, while the start is
>> still a ptrdiff (verify_gimple ICE, boom).
>>
>> The attached patch converts `start' to an integer_type to match the length
>> and stride.  We could either do this, or do a fold_convert if
>> !useless_type_conversion_p in create_array_refs.  I didn't want to look into
>> cilkplus too deeply as to why we have different types, because (a) I don't
>> care (b) we're probably going to deprecate Cilk Plus, no?
>
> Conceptually, using integer_type_node for these things is complete wrong,
> unless the Cilk+ specification says that all the array notation expressions
> are converted to int.  Because forcing the int there means that it will
> misbehave on very large arrays (over 2GB elements).
> So much better would be to have the expressions converted to sizetype or
> something similar that the middle-end works with (yes, it is unsigned, so
> if it needs to be signed somewhere, we'd need corresponding signed type for
> that).
>
> The question is where all is the integer_type_node in the Cilk+ lowering
> hardcoded and how hard would it be to fix it up.
>
> If it is too hard, I guess I can live with this patch, but there should be a
> PR that it needs to be fixed not to hardcode int type which is inappropriate
> for sizes/lengths.

As discussed on IRC, we will probably deprecate CilkPlus for GCC7 and 
remove it for GCC8 unless someone is interested in maintaining it. 
So...committing as is.

>
> And the more important question is if Intel is willing to maintain Cilk+ in
> GCC, or if we should deprecate it (and, if the latter, if already in GCC7
> deprecate, remove in GCC8, or deprecate in GCC8, remove in GCC9).
> There are various Cilk+ related PRs around on which nothing has been done
> for many months.

Aldy