| Message ID | 6cf1a928-c7c3-8e7b-aff6-673ec8f01b2f@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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); +}
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? 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.