From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37992 invoked by alias); 30 Oct 2015 13:38:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 37967 invoked by uid 89); 30 Oct 2015 13:38:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 30 Oct 2015 13:38:02 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 97D5DC0A526D; Fri, 30 Oct 2015 13:38:00 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-121.ams2.redhat.com [10.36.116.121]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9UDbwYI030297 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 30 Oct 2015 09:37:59 -0400 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id t9UDbuMX023289; Fri, 30 Oct 2015 14:37:56 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id t9UDbsP2023288; Fri, 30 Oct 2015 14:37:54 +0100 Date: Fri, 30 Oct 2015 13:41:00 -0000 From: Jakub Jelinek To: Cesar Philippidis , Jason Merrill Cc: "gcc-patches@gcc.gnu.org" , Nathan Sidwell Subject: Re: [OpenACC] num_gangs, num_workers and vector_length in c++ Message-ID: <20151030133753.GA478@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <5632A573.80002@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5632A573.80002@codesourcery.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg03399.txt.bz2 On Thu, Oct 29, 2015 at 04:02:11PM -0700, Cesar Philippidis wrote: > I noticed that num_gangs, num_workers and vector_length are parsed in > somewhat insistent ways in the c++ FE. Both vector_length and num_gangs > bail out whenever as soon as they detect errors, whereas num_workers > does not. Besides for that, they are also checking for integral > expressions as the arguments are scanned instead of deferring that check > to finish_omp_clauses. That check will cause ICEs when template > arguments are used when we add support for template arguments later on. > > Rather than fix each function individually, I've consolidated them into > a single cp_parser_oacc_positive_int_clause function. While this > function could be extended to support openmp clauses which accept an > integer expression argument, like num_threads, I've decided to leave > those as-is since there are no known problems with those functions at > this moment. First question is what int-expr in OpenACC actually stands for (but I'll have to raise similar question for OpenMP too). Previously you were using cp_parser_condition, which is clearly undesirable in this case, it allows e.g. num_gangs (int a = 5) but the question is if num_gangs (5, 6) is valid and stands for (5, 6) expression, then it should use cp_parser_expression, or if you want to error on it, then you should use cp_parser_assignment_expression. >From quick skimming of the (now removed) C/C++ Grammar Appendix in OpenMP, I believe all the places where expression or scalar-expression is used in the grammar are meant to be cp_parser_expression cases (except expression-list used in UDRs which stands for normal C++ expression-list non-terminal), so clearly I need to fix up omp_clause_{if,final} to call cp_parser_expression instead of cp_parser_condition, and the various OpenMP clauses that use cp_parser_assignment_expression to instead use cp_parser_expression. Say schedule(static, 3, 6) should be valid IMHO. But, in OpenMP expression or scalar-expression in the grammar is never followed by , or optional , while in OpenACC grammar clearly is (e.g. for the gang clause). If OpenACC wants something different, clearly you can't share the parsing routines between say num_tasks and num_workers. Another thing is what Jason as C++ maintainer wants, it is nice to get rid of some code redundancies, on the other side the fact that there is one function per non-terminal in the grammar is also quite nice property. I know I've violated this a few times too. Next question is, why do you call it cp_parser_oacc_positive_int_clause when the parsing function actually doesn't verify neither the positive nor the int properties (and it should not), so perhaps it should just reflect in the name that it is a clause with assignment? expression. Or, see the previous paragraph, have a helper that does that and then have a separate function for each clause kind that calls those with the right arguments. Jakub