| Message ID | 1451576417-8710-1-git-send-email-patrick@parcs.ath.cx |
|---|---|
| State | New |
| Headers | show |
On 12/31/2015 08:40 AM, Patrick Palka wrote: > The Cilk Plus code may sometimes turn a COND_EXPR into an > error_mark_node for no good reason. This can be seen by compiling the > test case c-c++-common/cilk-plus/CK/pr60469.c with both gcc and g++ and > observing the differences of the -fdump-tree-original dumps: > > With gcc, we get the following code in the GENERIC dump: > > finally > { > _Cilk_sync; > D.1897.worker->current_stack_frame = D.1897.call_parent; > __cilkrts_pop_frame (&D.1897); > if (D.1897.flags != 16777216) > { > __cilkrts_leave_frame (&D.1897); > } > } > > Whereas with g++ we get > > finally > { > _Cilk_sync; > D.2387.worker->current_stack_frame = D.2387.call_parent; > __cilkrts_pop_frame (&D.2387); > <<< error >>> > } > > Notice that the COND_EXPR is replaced with an << error >> in the g++ > dump. > > This patch fixes the COND_EXPR handling within Cilk Plus. I think the > cause is a simple typo/logic bug. > > gcc/cp/ChangeLog: > > * cp-array-notation.c (cp_expand_cond_array_notations): Return > error_mark_node only if find_rank failed, not if it was > successful. Can you use -fdump-tree-original in the testcase and verify there's no <<< error >>> expressions in the resulting dump file? With that change, this is OK. jeff
On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote: > >gcc/cp/ChangeLog: > > > > * cp-array-notation.c (cp_expand_cond_array_notations): Return > > error_mark_node only if find_rank failed, not if it was > > successful. > Can you use -fdump-tree-original in the testcase and verify there's no <<< > error >>> expressions in the resulting dump file? > > With that change, this is OK. I think the patch is incomplete. Because, find_rank does not always emit an error if it returns false, so we again have cases where we can get error_mark_node in the code without error being emitted. else if (*rank != current_rank) { /* In this case, find rank is being recursed through a set of expression of the form A <OPERATION> B, where A and B both have array notations in them and the rank of A is not equal to rank of B. A simple example of such case is the following: X[:] + Y[:][:] */ *rank = current_rank; return false; } and other spots. E.g. if (prev_arg && EXPR_HAS_LOCATION (prev_arg)) error_at (EXPR_LOCATION (prev_arg), "rank mismatch between %qE and %qE", prev_arg, TREE_OPERAND (expr, ii)); looks very suspicious. Jakub
On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote: >> >gcc/cp/ChangeLog: >> > >> > * cp-array-notation.c (cp_expand_cond_array_notations): Return >> > error_mark_node only if find_rank failed, not if it was >> > successful. >> Can you use -fdump-tree-original in the testcase and verify there's no <<< >> error >>> expressions in the resulting dump file? >> >> With that change, this is OK. > > I think the patch is incomplete. Because, find_rank does not always emit > an error if it returns false, so we again have cases where we can get > error_mark_node in the code without error being emitted. > else if (*rank != current_rank) > { > /* In this case, find rank is being recursed through a set of > expression of the form A <OPERATION> B, where A and B both have > array notations in them and the rank of A is not equal to rank of B. > A simple example of such case is the following: X[:] + Y[:][:] */ > *rank = current_rank; > return false; > } > and other spots. E.g. > if (prev_arg && EXPR_HAS_LOCATION (prev_arg)) > error_at (EXPR_LOCATION (prev_arg), > "rank mismatch between %qE and %qE", prev_arg, > TREE_OPERAND (expr, ii)); > looks very suspicious. Hmm, good point. Here's a contrived test case that causes find_rank to return false without emitting an error message thus we again end up with an error_mark_node in the gimplifier: /* { dg-do compile } */ /* { dg-options "-fcilkplus" } */ void foo() {} #define ALEN 1024 int main(int argc, char* argv[]) { typedef void (*f) (void *); f b[ALEN], c[ALEN][ALEN]; (b[:]) ((void *)c[:][:]); _Cilk_spawn foo(); return 0; } But this patch was intended to only fix the testsuite fallout that patch 3 would have otherwise caused, and not to e.g. fix all the bugs with find_rank. (BTW patch 3 also makes this test case trigger an ICE, instead of being silently miscompiled.)
On 01/02/2016 04:26 PM, Patrick Palka wrote: > On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote: >>>> gcc/cp/ChangeLog: >>>> >>>> * cp-array-notation.c (cp_expand_cond_array_notations): Return >>>> error_mark_node only if find_rank failed, not if it was >>>> successful. >>> Can you use -fdump-tree-original in the testcase and verify there's no <<< >>> error >>> expressions in the resulting dump file? >>> >>> With that change, this is OK. >> >> I think the patch is incomplete. Because, find_rank does not always emit >> an error if it returns false, so we again have cases where we can get >> error_mark_node in the code without error being emitted. >> else if (*rank != current_rank) >> { >> /* In this case, find rank is being recursed through a set of >> expression of the form A <OPERATION> B, where A and B both have >> array notations in them and the rank of A is not equal to rank of B. >> A simple example of such case is the following: X[:] + Y[:][:] */ >> *rank = current_rank; >> return false; >> } >> and other spots. E.g. >> if (prev_arg && EXPR_HAS_LOCATION (prev_arg)) >> error_at (EXPR_LOCATION (prev_arg), >> "rank mismatch between %qE and %qE", prev_arg, >> TREE_OPERAND (expr, ii)); >> looks very suspicious. > > Hmm, good point. Here's a contrived test case that causes find_rank to > return false without emitting an error message thus we again end up > with an error_mark_node in the gimplifier: > > /* { dg-do compile } */ > /* { dg-options "-fcilkplus" } */ > > void foo() {} > > #define ALEN 1024 > > int main(int argc, char* argv[]) > { > typedef void (*f) (void *); > f b[ALEN], c[ALEN][ALEN]; > (b[:]) ((void *)c[:][:]); > _Cilk_spawn foo(); > return 0; > } > > But this patch was intended to only fix the testsuite fallout that > patch 3 would have otherwise caused, and not to e.g. fix all the bugs > with find_rank. > > (BTW patch 3 also makes this test case trigger an ICE, instead of > being silently miscompiled.) Can you please include this test (xfailed) when you commit patch #1. I think you want the test to scan for error_mark_node in the gimplified dump. Jeff
On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote: > On 01/02/2016 04:26 PM, Patrick Palka wrote: >> >> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> >>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote: >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * cp-array-notation.c (cp_expand_cond_array_notations): Return >>>>> error_mark_node only if find_rank failed, not if it was >>>>> successful. >>>> >>>> Can you use -fdump-tree-original in the testcase and verify there's no >>>> <<< >>>> error >>> expressions in the resulting dump file? >>>> >>>> With that change, this is OK. >>> >>> >>> I think the patch is incomplete. Because, find_rank does not always emit >>> an error if it returns false, so we again have cases where we can get >>> error_mark_node in the code without error being emitted. >>> else if (*rank != current_rank) >>> { >>> /* In this case, find rank is being recursed through a set of >>> expression of the form A <OPERATION> B, where A and B both >>> have >>> array notations in them and the rank of A is not equal to >>> rank of B. >>> A simple example of such case is the following: X[:] + >>> Y[:][:] */ >>> *rank = current_rank; >>> return false; >>> } >>> and other spots. E.g. >>> if (prev_arg && EXPR_HAS_LOCATION (prev_arg)) >>> error_at (EXPR_LOCATION (prev_arg), >>> "rank mismatch between %qE and %qE", >>> prev_arg, >>> TREE_OPERAND (expr, ii)); >>> looks very suspicious. >> >> >> Hmm, good point. Here's a contrived test case that causes find_rank to >> return false without emitting an error message thus we again end up >> with an error_mark_node in the gimplifier: >> >> /* { dg-do compile } */ >> /* { dg-options "-fcilkplus" } */ >> >> void foo() {} >> >> #define ALEN 1024 >> >> int main(int argc, char* argv[]) >> { >> typedef void (*f) (void *); >> f b[ALEN], c[ALEN][ALEN]; >> (b[:]) ((void *)c[:][:]); >> _Cilk_spawn foo(); >> return 0; >> } >> >> But this patch was intended to only fix the testsuite fallout that >> patch 3 would have otherwise caused, and not to e.g. fix all the bugs >> with find_rank. >> >> (BTW patch 3 also makes this test case trigger an ICE, instead of >> being silently miscompiled.) > > Can you please include this test (xfailed) when you commit patch #1. I > think you want the test to scan for error_mark_node in the gimplified dump. There are four subdirectories under gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which directory should this new xfailed test go? > > Jeff >
On 01/10/2016 08:55 PM, Patrick Palka wrote: > On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote: >> On 01/02/2016 04:26 PM, Patrick Palka wrote: >>> >>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> >>>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote: >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> * cp-array-notation.c (cp_expand_cond_array_notations): Return >>>>>> error_mark_node only if find_rank failed, not if it was >>>>>> successful. >>>>> >>>>> Can you use -fdump-tree-original in the testcase and verify there's no >>>>> <<< >>>>> error >>> expressions in the resulting dump file? >>>>> >>>>> With that change, this is OK. >>>> >>>> >>>> I think the patch is incomplete. Because, find_rank does not always emit >>>> an error if it returns false, so we again have cases where we can get >>>> error_mark_node in the code without error being emitted. >>>> else if (*rank != current_rank) >>>> { >>>> /* In this case, find rank is being recursed through a set of >>>> expression of the form A <OPERATION> B, where A and B both >>>> have >>>> array notations in them and the rank of A is not equal to >>>> rank of B. >>>> A simple example of such case is the following: X[:] + >>>> Y[:][:] */ >>>> *rank = current_rank; >>>> return false; >>>> } >>>> and other spots. E.g. >>>> if (prev_arg && EXPR_HAS_LOCATION (prev_arg)) >>>> error_at (EXPR_LOCATION (prev_arg), >>>> "rank mismatch between %qE and %qE", >>>> prev_arg, >>>> TREE_OPERAND (expr, ii)); >>>> looks very suspicious. >>> >>> >>> Hmm, good point. Here's a contrived test case that causes find_rank to >>> return false without emitting an error message thus we again end up >>> with an error_mark_node in the gimplifier: >>> >>> /* { dg-do compile } */ >>> /* { dg-options "-fcilkplus" } */ >>> >>> void foo() {} >>> >>> #define ALEN 1024 >>> >>> int main(int argc, char* argv[]) >>> { >>> typedef void (*f) (void *); >>> f b[ALEN], c[ALEN][ALEN]; >>> (b[:]) ((void *)c[:][:]); >>> _Cilk_spawn foo(); >>> return 0; >>> } >>> >>> But this patch was intended to only fix the testsuite fallout that >>> patch 3 would have otherwise caused, and not to e.g. fix all the bugs >>> with find_rank. >>> >>> (BTW patch 3 also makes this test case trigger an ICE, instead of >>> being silently miscompiled.) >> >> Can you please include this test (xfailed) when you commit patch #1. I >> think you want the test to scan for error_mark_node in the gimplified dump. > > There are four subdirectories under > gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which > directory should this new xfailed test go? These are for array notation, so AN seems best. Jeff
diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c index 8862af1..3a6d773 100644 --- a/gcc/cp/cp-array-notation.c +++ b/gcc/cp/cp-array-notation.c @@ -807,8 +807,8 @@ cp_expand_cond_array_notations (tree orig_stmt) if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank) || !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true, &yes_rank) - || find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true, - &no_rank)) + || !find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true, + &no_rank)) return error_mark_node; /* If the condition has a zero rank, then handle array notations in body separately. */
The Cilk Plus code may sometimes turn a COND_EXPR into an error_mark_node for no good reason. This can be seen by compiling the test case c-c++-common/cilk-plus/CK/pr60469.c with both gcc and g++ and observing the differences of the -fdump-tree-original dumps: With gcc, we get the following code in the GENERIC dump: finally { _Cilk_sync; D.1897.worker->current_stack_frame = D.1897.call_parent; __cilkrts_pop_frame (&D.1897); if (D.1897.flags != 16777216) { __cilkrts_leave_frame (&D.1897); } } Whereas with g++ we get finally { _Cilk_sync; D.2387.worker->current_stack_frame = D.2387.call_parent; __cilkrts_pop_frame (&D.2387); <<< error >>> } Notice that the COND_EXPR is replaced with an << error >> in the g++ dump. This patch fixes the COND_EXPR handling within Cilk Plus. I think the cause is a simple typo/logic bug. gcc/cp/ChangeLog: * cp-array-notation.c (cp_expand_cond_array_notations): Return error_mark_node only if find_rank failed, not if it was successful. --- gcc/cp/cp-array-notation.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)