From patchwork Thu Nov 17 09:34:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chung-Lin Tang X-Patchwork-Id: 82692 Delivered-To: patch@linaro.org Received: by 10.182.1.168 with SMTP id 8csp770131obn; Thu, 17 Nov 2016 01:35:13 -0800 (PST) X-Received: by 10.98.87.199 with SMTP id i68mr3532913pfj.18.1479375313069; Thu, 17 Nov 2016 01:35:13 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id z21si2486343pgc.193.2016.11.17.01.35.12 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 17 Nov 2016 01:35:13 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-441781-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-441781-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-441781-patch=linaro.org@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=JLfMC9F3nDY/FW8iz VXRelNS0Ckm1fhwD5ayulXWFpMR+aV/jGCBZRx88QEzaVkt5x7Y7b/unBhgEPy1+ ElIk0jcn+8/SjMuvBV+5tNIDUO2+7piMXbqr5ULDri+Dlvwjh3WLF61eibAOnB2J zAB6zruSS5BM0B8XaT24vPhYDs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=IIMEw/4mEtJ5+Gt37na5shc ecVE=; b=GUWT6kugV+UCqZvu0pPPVR0cyguHMkF8f2/INZUT/Po5k9en9+50P/a z+BwY0DHxG73OQBqbglgO15U1VpYhShdiG3BznihemMPhxQE7oE9lxtJmjsVJo64 IFY2mF7NOPgo4UWNbT8+noi0s3aFIKtJ4oHaU/fVbrLZ+erI2+G4= Received: (qmail 24083 invoked by alias); 17 Nov 2016 09:34:55 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 24069 invoked by uid 89); 17 Nov 2016 09:34:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=no version=3.3.2 spammy=sk:tree_fi, sk:fold_bu, fold_build2_loc, sk:check_n X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Nov 2016 09:34:44 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1c7J5e-00029b-UN from ChungLin_Tang@mentor.com ; Thu, 17 Nov 2016 01:34:42 -0800 Received: from svr-orw-mbx-04.mgc.mentorg.com (147.34.90.204) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 17 Nov 2016 01:34:40 -0800 Received: from [0.0.0.0] (147.34.91.1) by svr-orw-mbx-04.mgc.mentorg.com (147.34.90.204) with Microsoft SMTP Server (TLS) id 15.0.1210.3 via Frontend Transport; Thu, 17 Nov 2016 01:34:38 -0800 Subject: Re: [Patch 3/5] OpenACC tile clause support, C/C++ front-end parts To: Jakub Jelinek References: <61e744b3-3ae8-1709-df7b-52d65bbfb676@mentor.com> <20161111103038.GX3541@tucnak.redhat.com> CC: gcc-patches , Nathan Sidwell , Cesar Philippidis From: Chung-Lin Tang Message-ID: <56973ea6-3f8c-0035-8443-a14aa7b88437@mentor.com> Date: Thu, 17 Nov 2016 17:34:34 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161111103038.GX3541@tucnak.redhat.com> On 2016/11/11 6:30 PM, Jakub Jelinek wrote: > On Thu, Nov 10, 2016 at 06:46:16PM +0800, Chung-Lin Tang wrote: >> 2016-XX-XX Nathan Sidwell >> >> c/ >> * c-parser.c (c_parser_omp_clause_collapse): Disallow tile. >> (c_parser_oacc_clause_tile): Disallow collapse. Fix parsing and >> semantic checking. >> * c-parser.c (c_parser_omp_for_loop): Accept tiling constructs. >> >> cp/ >> * parser.c (cp_parser_oacc_clause_tile): Disallow collapse. Fix >> parsing. Parse constant expression. Remove semantic checking. >> (cp_parser_omp_clause_collapse): Disallow tile. >> (cp_parser_omp_for_loop): Deal with tile clause. Don't emit a > > Similarly to the previous patch, some lines have spaces instead of tabs. > >> parse error about missing for after already emitting one. >> Use more conventional for idiom for unbounded loop. >> * pt.c (tsubst_omp_clauses): Require integral constant expression >> for COLLAPSE and TILE. Remove broken TILE subst. >> * semantics.c (finish_omp_clauses): Correct TILE semantic check. >> (finish_omp_for): Deal with tile clause. >> >> gcc/testsuite/ >> * c-c++-common/goacc/loop-auto-1.c: Adjust and add additional >> case. >> * c-c++-common/goacc/loop-auto-2.c: New. >> * c-c++-common/goacc/tile.c: Include stdbool, fix expected errors. >> * g++.dg/goacc/template.C: Test tile subst. Adjust erroneous >> uses. >> * g++.dg/goacc/tile-1.C: Check tile subst. >> * gcc.dg/goacc/loop-processing-1.c: Adjust dg-final pattern. > >> + if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)) >> + || TREE_CODE (expr) != INTEGER_CST > > No need to test for INTEGER_CST, tree_fits_shwi_p will test that. > >> + || !tree_fits_shwi_p (expr) >> + || tree_to_shwi (expr) <= 0) >> { >> - warning_at (expr_loc, 0,"% value must be positive"); >> - expr = integer_one_node; >> + error_at (expr_loc, "% argument needs positive" >> + " integral constant"); >> + expr = integer_zero_node; >> } >> } > >> @@ -14713,6 +14713,7 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio >> nc = copy_node (oc); >> OMP_CLAUSE_CHAIN (nc) = new_clauses; >> new_clauses = nc; >> + bool needs_ice = false; >> >> switch (OMP_CLAUSE_CODE (nc)) >> { >> @@ -14742,10 +14743,16 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio >> = tsubst_omp_clause_decl (OMP_CLAUSE_DECL (oc), args, complain, >> in_decl); >> break; >> + case OMP_CLAUSE_COLLAPSE: >> + case OMP_CLAUSE_TILE: >> + /* These clauses really need a positive integral constant >> + expression, but we don't have a predicate for that >> + (yet). */ >> + needs_ice = true; >> + /* FALLTHRU */ > > As I said earlier on gcc-patches, no need to change anything for > OMP_CLAUSE_COLLAPSE, we require that the argument is a constant integer > already at parsing time, it can't be e.g. a template integral parameter. > And for OMP_CLAUSE_TILE, please avoid the needs_ice var, instead don't fall > through into the tsubst_expr and copy it over and change the argument there > instead, it is short enough. > >> + if (TREE_CODE (t) != INTEGER_CST >> + || !tree_fits_shwi_p (t) > > Again, no need to check for INTEGER_CST when tree_fits_shwi_p will do that. > > Jakub Updated C/C++ front-end patches, adjusted as reviewed. Thanks, Chung-Lin Index: c/c-parser.c =================================================================== --- c/c-parser.c (revision 241809) +++ c/c-parser.c (working copy) @@ -11010,6 +11010,7 @@ c_parser_omp_clause_collapse (c_parser *parser, tr location_t loc; check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse"); + check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile"); loc = c_parser_peek_token (parser)->location; if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) @@ -11920,10 +11921,11 @@ static tree c_parser_oacc_clause_tile (c_parser *parser, tree list) { tree c, expr = error_mark_node; - location_t loc, expr_loc; + location_t loc; tree tile = NULL_TREE; check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile"); + check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse"); loc = c_parser_peek_token (parser)->location; if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) @@ -11931,16 +11933,19 @@ c_parser_oacc_clause_tile (c_parser *parser, tree do { + if (tile && !c_parser_require (parser, CPP_COMMA, "expected %<,%>")) + return list; + if (c_parser_next_token_is (parser, CPP_MULT) && (c_parser_peek_2nd_token (parser)->type == CPP_COMMA || c_parser_peek_2nd_token (parser)->type == CPP_CLOSE_PAREN)) { c_parser_consume_token (parser); - expr = integer_minus_one_node; + expr = integer_zero_node; } else { - expr_loc = c_parser_peek_token (parser)->location; + location_t expr_loc = c_parser_peek_token (parser)->location; c_expr cexpr = c_parser_expr_no_commas (parser, NULL); cexpr = convert_lvalue_to_rvalue (expr_loc, cexpr, false, true); expr = cexpr.value; @@ -11952,28 +11957,19 @@ c_parser_oacc_clause_tile (c_parser *parser, tree return list; } - if (!INTEGRAL_TYPE_P (TREE_TYPE (expr))) - { - c_parser_error (parser, "% value must be integral"); - return list; - } - expr = c_fully_fold (expr, false, NULL); - /* Attempt to statically determine when expr isn't positive. */ - c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, expr, - build_int_cst (TREE_TYPE (expr), 0)); - protected_set_expr_location (c, expr_loc); - if (c == boolean_true_node) + if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)) + || !tree_fits_shwi_p (expr) + || tree_to_shwi (expr) <= 0) { - warning_at (expr_loc, 0,"% value must be positive"); - expr = integer_one_node; + error_at (expr_loc, "% argument needs positive" + " integral constant"); + expr = integer_zero_node; } } tile = tree_cons (NULL_TREE, expr, tile); - if (c_parser_next_token_is (parser, CPP_COMMA)) - c_parser_consume_token (parser); } while (c_parser_next_token_is_not (parser, CPP_CLOSE_PAREN)); @@ -14899,11 +14895,17 @@ c_parser_omp_for_loop (location_t loc, c_parser *p bool fail = false, open_brace_parsed = false; int i, collapse = 1, ordered = 0, count, nbraces = 0; location_t for_loc; + bool tiling = false; vec *for_block = make_tree_vector (); for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl)) if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_COLLAPSE) collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl)); + else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_TILE) + { + tiling = true; + collapse = list_length (OMP_CLAUSE_TILE_LIST (cl)); + } else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_ORDERED && OMP_CLAUSE_ORDERED_EXPR (cl)) { @@ -14933,7 +14935,7 @@ c_parser_omp_for_loop (location_t loc, c_parser *p pc = &OMP_CLAUSE_CHAIN (*pc); } - gcc_assert (collapse >= 1 && ordered >= 0); + gcc_assert (tiling || (collapse >= 1 && ordered >= 0)); count = ordered ? ordered : collapse; declv = make_tree_vec (count); Index: cp/pt.c =================================================================== --- cp/pt.c (revision 241809) +++ cp/pt.c (working copy) @@ -14742,6 +14742,13 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio = tsubst_omp_clause_decl (OMP_CLAUSE_DECL (oc), args, complain, in_decl); break; + case OMP_CLAUSE_TILE: + /* These clauses really need a positive integral constant expression, + but we don't have a predicate for that (yet). */ + OMP_CLAUSE_OPERAND (nc, 0) + = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, in_decl, + /*integral_constant_expression_p=*/true); + break; case OMP_CLAUSE_IF: case OMP_CLAUSE_NUM_THREADS: case OMP_CLAUSE_SCHEDULE: @@ -14766,7 +14773,7 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio case OMP_CLAUSE_ASYNC: case OMP_CLAUSE_WAIT: OMP_CLAUSE_OPERAND (nc, 0) - = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, + = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, in_decl, /*integral_constant_expression_p=*/false); break; case OMP_CLAUSE_REDUCTION: @@ -14836,19 +14843,6 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio case OMP_CLAUSE_AUTO: case OMP_CLAUSE_SEQ: break; - case OMP_CLAUSE_TILE: - { - tree lnc, loc; - for (lnc = OMP_CLAUSE_TILE_LIST (nc), - loc = OMP_CLAUSE_TILE_LIST (oc); - loc; - loc = TREE_CHAIN (loc), lnc = TREE_CHAIN (lnc)) - { - TREE_VALUE (lnc) = tsubst_expr (TREE_VALUE (loc), args, - complain, in_decl, false); - } - } - break; default: gcc_unreachable (); } Index: cp/semantics.c =================================================================== --- cp/semantics.c (revision 241809) +++ cp/semantics.c (working copy) @@ -7086,7 +7086,8 @@ finish_omp_clauses (tree clauses, enum c_omp_regio else if (!type_dependent_expression_p (t) && !INTEGRAL_TYPE_P (TREE_TYPE (t))) { - error ("% value must be integral"); + error_at (OMP_CLAUSE_LOCATION (c), + "% argument needs integral type"); remove = true; } else @@ -7094,14 +7095,16 @@ finish_omp_clauses (tree clauses, enum c_omp_regio t = mark_rvalue_use (t); if (!processing_template_decl) { + /* Zero is used to indicate '*', we permit you + to get there via an ICE of value zero. */ t = maybe_constant_value (t); - if (TREE_CODE (t) == INTEGER_CST - && tree_int_cst_sgn (t) != 1 - && t != integer_minus_one_node) + if (!tree_fits_shwi_p (t) + || tree_to_shwi (t) < 0) { - warning_at (OMP_CLAUSE_LOCATION (c), 0, - "% value must be positive"); - t = integer_one_node; + error_at (OMP_CLAUSE_LOCATION (c), + "% argument needs positive " + "integral constant"); + remove = true; } } t = fold_build_cleanup_point_expr (TREE_TYPE (t), t); @@ -8000,11 +8003,19 @@ finish_omp_for (location_t locus, enum tree_code c gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (incrv)); if (TREE_VEC_LENGTH (declv) > 1) { - tree c = find_omp_clause (clauses, OMP_CLAUSE_COLLAPSE); + tree c; + + c = find_omp_clause (clauses, OMP_CLAUSE_TILE); if (c) - collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (c)); - if (collapse != TREE_VEC_LENGTH (declv)) - ordered = TREE_VEC_LENGTH (declv); + collapse = list_length (OMP_CLAUSE_TILE_LIST (c)); + else + { + c = find_omp_clause (clauses, OMP_CLAUSE_COLLAPSE); + if (c) + collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (c)); + if (collapse != TREE_VEC_LENGTH (declv)) + ordered = TREE_VEC_LENGTH (declv); + } } for (i = 0; i < TREE_VEC_LENGTH (declv); i++) { Index: cp/parser.c =================================================================== --- cp/parser.c (revision 241809) +++ cp/parser.c (working copy) @@ -30885,30 +30885,33 @@ cp_parser_oacc_clause_tile (cp_parser *parser, loc tree c, expr = error_mark_node; tree tile = NULL_TREE; + /* Collapse and tile are mutually exclusive. (The spec doesn't say + so, but the spec authors never considered such a case and have + differing opinions on what it might mean, including 'not + allowed'.) */ check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile", clause_loc); + check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse", + clause_loc); if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN)) return list; do { + if (tile && !cp_parser_require (parser, CPP_COMMA, RT_COMMA)) + return list; + if (cp_lexer_next_token_is (parser->lexer, CPP_MULT) && (cp_lexer_nth_token_is (parser->lexer, 2, CPP_COMMA) || cp_lexer_nth_token_is (parser->lexer, 2, CPP_CLOSE_PAREN))) { cp_lexer_consume_token (parser->lexer); - expr = integer_minus_one_node; + expr = integer_zero_node; } else - expr = cp_parser_assignment_expression (parser, NULL, false, false); + expr = cp_parser_constant_expression (parser); - if (expr == error_mark_node) - return list; - tile = tree_cons (NULL_TREE, expr, tile); - - if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA)) - cp_lexer_consume_token (parser->lexer); } while (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_PAREN)); @@ -31021,6 +31024,7 @@ cp_parser_omp_clause_collapse (cp_parser *parser, } check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse", location); + check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile", location); c = build_omp_clause (loc, OMP_CLAUSE_COLLAPSE); OMP_CLAUSE_CHAIN (c) = list; OMP_CLAUSE_COLLAPSE_EXPR (c) = num; @@ -34027,10 +34031,16 @@ cp_parser_omp_for_loop (cp_parser *parser, enum tr int i, collapse = 1, ordered = 0, count, nbraces = 0; vec *for_block = make_tree_vector (); auto_vec orig_inits; + bool tiling = false; for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl)) if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_COLLAPSE) collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl)); + else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_TILE) + { + tiling = true; + collapse = list_length (OMP_CLAUSE_TILE_LIST (cl)); + } else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_ORDERED && OMP_CLAUSE_ORDERED_EXPR (cl)) { @@ -34060,7 +34070,7 @@ cp_parser_omp_for_loop (cp_parser *parser, enum tr pc = &OMP_CLAUSE_CHAIN (*pc); } - gcc_assert (collapse >= 1 && ordered >= 0); + gcc_assert (tiling || (collapse >= 1 && ordered >= 0)); count = ordered ? ordered : collapse; declv = make_tree_vec (count); @@ -34079,13 +34089,15 @@ cp_parser_omp_for_loop (cp_parser *parser, enum tr if (code != CILK_FOR && !cp_lexer_next_token_is_keyword (parser->lexer, RID_FOR)) { - cp_parser_error (parser, "for statement expected"); + if (!collapse_err) + cp_parser_error (parser, "for statement expected"); return NULL; } if (code == CILK_FOR && !cp_lexer_next_token_is_keyword (parser->lexer, RID_CILK_FOR)) { - cp_parser_error (parser, "_Cilk_for statement expected"); + if (!collapse_err) + cp_parser_error (parser, "_Cilk_for statement expected"); return NULL; } loc = cp_lexer_consume_token (parser->lexer)->location; @@ -34245,7 +34257,7 @@ cp_parser_omp_for_loop (cp_parser *parser, enum tr nested. Hopefully the final version clarifies this. For now handle (multiple) {'s and empty statements. */ cp_parser_parse_tentatively (parser); - do + for (;;) { if (cp_lexer_next_token_is_keyword (parser->lexer, RID_FOR)) break; @@ -34260,14 +34272,13 @@ cp_parser_omp_for_loop (cp_parser *parser, enum tr else { loc = cp_lexer_peek_token (parser->lexer)->location; - error_at (loc, "not enough collapsed for loops"); + error_at (loc, "not enough for loops to collapse"); collapse_err = true; cp_parser_abort_tentative_parse (parser); declv = NULL_TREE; break; } } - while (1); if (declv) {