From patchwork Thu Oct 10 08:13:28 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 1174357 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-510595-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="yhezg/0E"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 46pkPQ6hd5z9sCJ for ; Thu, 10 Oct 2019 19:13:45 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=iouqcg71IN4ydcsB/ 2LUoVDDcnCSiURuxlAUHU7IEiQDjoswsnJqFhEZdlpdQK1q/zdK2Ildie9OfdgEq ppUofloxMZA+uHezMi87R+AXdxvqVXoyzTzZp8zPy/Gu6oyRXAUlGXwaCN+HpCWb Y8o1Y7/ouR/sRv2OwQpjUER9vA= 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:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; s=default; bh=lTavb7jXjHyrNTz40B+iX0h eWm8=; b=yhezg/0EDzyGhQHQllxqg2SbW9UZbwjT0xHomQnh/KZOmWxhSMkhXxc 9izi2Bs1D6bsmicIaXI+icmjgL9Hq47Zu5BOqn8h0eQWgdMrbZORmC00gMeFxDem H66AKUjAQqzz5QxhbH0zn9W+M+X0fCGuMb6iItnNe3w0dGQ9AhE0= Received: (qmail 62921 invoked by alias); 10 Oct 2019 08:13:37 -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 62911 invoked by uid 89); 10 Oct 2019 08:13:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-10.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=tre, H*Ad:U*nathan, H*f:sk:e13cccf, sk:is_gimp 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 ESMTP; Thu, 10 Oct 2019 08:13:34 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 560D9302C07C; Thu, 10 Oct 2019 08:13:33 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-90.ams2.redhat.com [10.36.116.90]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E0B5E600F8; Thu, 10 Oct 2019 08:13:32 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id x9A8DUYx011519; Thu, 10 Oct 2019 10:13:31 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id x9A8DSlR011518; Thu, 10 Oct 2019 10:13:28 +0200 Date: Thu, 10 Oct 2019 10:13:28 +0200 From: Jakub Jelinek To: Jason Merrill Cc: Nathan Sidwell , gcc-patches@gcc.gnu.org Subject: [C++ PATCH] Partial fix for further -fstrong-eval-order issues (PR c++/91987, take 2) Message-ID: <20191010081328.GI15914@tucnak> Reply-To: Jakub Jelinek References: <20191007162315.GM15914@tucnak> <0a024ffb-7141-bbc9-4d0a-a5eea5404662@redhat.com> <20191007201046.GO15914@tucnak> <20191007205756.GP15914@tucnak> <7f52e8b2-e968-9edc-1640-f04d81d4e13d@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7f52e8b2-e968-9edc-1640-f04d81d4e13d@redhat.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-IsSubscribed: yes On Mon, Oct 07, 2019 at 05:17:56PM -0400, Jason Merrill wrote: > Ah, I see. And it occurs to me this situation fails condition #1 for > get_formal_tmp_var anyway. So I guess the predicate direction doesn't make > sense. Let's factor out the above pattern differently, then. Maybe a > preevaluate function that takes a predicate as an argument? Ok, so this patch does, compared to the previous one: 1) shifts are now done in cp_build_binary_op with the cp_save_expr, so no fold-const.c or cp-gimplify.c change is needed 2) the array ref x[y] -> *(x + y) where y has side-effects is now handled as *(SAVE_EXPR, SAVE_EXPR + y) rather than SAVE_EXPR, *(SAVE_EXPR + y) where the former is friendlier to wrapping it in ADDR_EXPR etc.; nevertheless, I had to disable it for the OpenMP reductions, I'll discuss in the language committee what to do if anything in that case 3) I've added the new function in cp-gimplify.c and used it in the CALL_EXPR case Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? If wanted, can commit the 3 unrelated changes separately. And the call argument side-effects aren't handled yet. 2019-10-10 Jakub Jelinek PR c++/91987 cp/ * decl2.c (grok_array_decl): For -fstrong-eval-order, when array ref operands have been swapped and at least one operand has side-effects, revert the swapping before calling build_array_ref. * typeck.c (cp_build_array_ref): For non-ARRAY_TYPE array ref with side-effects on the index operand, if -fstrong-eval-order use save_expr around the array operand. (cp_build_binary_op): For shifts with side-effects in the second operand, wrap first operand into SAVE_EXPR and evaluate it before the shift. * semantics.c (handle_omp_array_sections_1): Temporarily disable flag_strong_eval_order during OMP_CLAUSE_REDUCTION array section processing. * cp-gimplify.c (gimplify_to_rvalue): New function. (cp_gimplify_expr): Use it. testsuite/ * g++.dg/cpp1z/eval-order6.C: New test. * g++.dg/cpp1z/eval-order7.C: New test. * g++.dg/cpp1z/eval-order8.C: New test. * c-c++-common/gomp/pr91987.c: New test. Jakub --- gcc/cp/decl2.c.jj 2019-10-09 10:27:12.704400889 +0200 +++ gcc/cp/decl2.c 2019-10-09 10:32:44.932416373 +0200 @@ -405,6 +405,7 @@ grok_array_decl (location_t loc, tree ar else { tree p1, p2, i1, i2; + bool swapped = false; /* Otherwise, create an ARRAY_REF for a pointer or array type. It is a little-known fact that, if `a' is an array and `i' is @@ -431,7 +432,7 @@ grok_array_decl (location_t loc, tree ar if (p1 && i2) array_expr = p1, index_exp = i2; else if (i1 && p2) - array_expr = p2, index_exp = i1; + swapped = true, array_expr = p2, index_exp = i1; else { error_at (loc, "invalid types %<%T[%T]%> for array subscript", @@ -447,7 +448,12 @@ grok_array_decl (location_t loc, tree ar else array_expr = mark_lvalue_use_nonread (array_expr); index_exp = mark_rvalue_use (index_exp); - expr = build_array_ref (input_location, array_expr, index_exp); + if (swapped + && flag_strong_eval_order == 2 + && (TREE_SIDE_EFFECTS (array_expr) || TREE_SIDE_EFFECTS (index_exp))) + expr = build_array_ref (input_location, index_exp, array_expr); + else + expr = build_array_ref (input_location, array_expr, index_exp); } if (processing_template_decl && expr != error_mark_node) { --- gcc/cp/typeck.c.jj 2019-10-09 10:27:12.625402077 +0200 +++ gcc/cp/typeck.c 2019-10-09 11:21:09.058765959 +0200 @@ -3550,6 +3550,10 @@ cp_build_array_ref (location_t loc, tree { tree ar = cp_default_conversion (array, complain); tree ind = cp_default_conversion (idx, complain); + tree first = NULL_TREE; + + if (flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind)) + ar = first = cp_save_expr (ar); /* Put the integer in IND to simplify error checking. */ if (TREE_CODE (TREE_TYPE (ar)) == INTEGER_TYPE) @@ -3573,11 +3577,10 @@ cp_build_array_ref (location_t loc, tree warn_array_subscript_with_type_char (loc, idx); - ret = cp_build_indirect_ref (cp_build_binary_op (input_location, - PLUS_EXPR, ar, ind, - complain), - RO_ARRAY_INDEXING, - complain); + ret = cp_build_binary_op (input_location, PLUS_EXPR, ar, ind, complain); + if (first) + ret = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (ret), first, ret); + ret = cp_build_indirect_ref (ret, RO_ARRAY_INDEXING, complain); protected_set_expr_location (ret, loc); if (non_lvalue) ret = non_lvalue_loc (loc, ret); @@ -5555,6 +5558,17 @@ cp_build_binary_op (const op_location_t if (build_type == NULL_TREE) build_type = result_type; + if (doing_shift + && flag_strong_eval_order == 2 + && TREE_SIDE_EFFECTS (op1) + && !processing_template_decl) + { + /* In C++17, in both op0 << op1 and op0 >> op1 op0 is sequenced before + op1, so if op1 has side-effects, use SAVE_EXPR around op0. */ + op0 = cp_save_expr (op0); + instrument_expr = op0; + } + if (sanitize_flags_p ((SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE)) && current_function_decl != NULL_TREE @@ -5566,6 +5580,7 @@ cp_build_binary_op (const op_location_t op1 = cp_save_expr (op1); op0 = fold_non_dependent_expr (op0, complain); op1 = fold_non_dependent_expr (op1, complain); + tree instrument_expr1 = NULL_TREE; if (doing_div_or_mod && sanitize_flags_p (SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE)) { @@ -5578,10 +5593,15 @@ cp_build_binary_op (const op_location_t cop0 = cp_convert (orig_type, op0, complain); if (TREE_TYPE (cop1) != orig_type) cop1 = cp_convert (orig_type, op1, complain); - instrument_expr = ubsan_instrument_division (location, cop0, cop1); + instrument_expr1 = ubsan_instrument_division (location, cop0, cop1); } else if (doing_shift && sanitize_flags_p (SANITIZE_SHIFT)) - instrument_expr = ubsan_instrument_shift (location, code, op0, op1); + instrument_expr1 = ubsan_instrument_shift (location, code, op0, op1); + if (instrument_expr != NULL) + instrument_expr = build2 (COMPOUND_EXPR, TREE_TYPE (instrument_expr1), + instrument_expr, instrument_expr1); + else + instrument_expr = instrument_expr1; } result = build2_loc (location, resultcode, build_type, op0, op1); --- gcc/cp/semantics.c.jj 2019-09-27 22:13:18.336903781 +0200 +++ gcc/cp/semantics.c 2019-10-09 12:25:46.852235717 +0200 @@ -5017,6 +5017,15 @@ handle_omp_array_sections_1 (tree c, tre TREE_PURPOSE (t) = lb; low_bound = lb; } + /* Temporarily disable -fstrong-eval-order for array reductions. + The SAVE_EXPR and COMPOUND_EXPR added if low_bound has side-effects + is something the middle-end can't cope with and more importantly, + it needs to be the actual base variable that is privatized, not some + temporary assigned previous value of it. That, together with OpenMP + saying how many times the side-effects are evaluated is unspecified, + makes int *a, *b; ... reduction(+:a[a = b, 3:10]) really unspecified. */ + warning_sentinel s (flag_strong_eval_order, + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION); ret = grok_array_decl (OMP_CLAUSE_LOCATION (c), ret, low_bound, false); return ret; } --- gcc/cp/cp-gimplify.c.jj 2019-10-09 10:27:12.654401641 +0200 +++ gcc/cp/cp-gimplify.c 2019-10-09 10:32:44.930416402 +0200 @@ -638,6 +638,22 @@ lvalue_has_side_effects (tree e) return TREE_SIDE_EFFECTS (e); } +/* Gimplify *EXPR_P as rvalue into an expression that can't be modified + by expressions with side-effects in other operands. */ + +static enum gimplify_status +gimplify_to_rvalue (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, + bool (*gimple_test_f) (tree)) +{ + enum gimplify_status t + = gimplify_expr (expr_p, pre_p, post_p, gimple_test_f, fb_rvalue); + if (t == GS_ERROR) + return GS_ERROR; + else if (is_gimple_variable (*expr_p) && TREE_CODE (*expr_p) != SSA_NAME) + *expr_p = get_initialized_tmp_var (*expr_p, pre_p, NULL); + return t; +} + /* Do C++-specific gimplification. Args are as for gimplify_expr. */ int @@ -823,15 +839,10 @@ cp_gimplify_expr (tree *expr_p, gimple_s && cp_get_callee_fndecl_nofold (*expr_p) == NULL_TREE) { enum gimplify_status t - = gimplify_expr (&CALL_EXPR_FN (*expr_p), pre_p, NULL, - is_gimple_call_addr, fb_rvalue); + = gimplify_to_rvalue (&CALL_EXPR_FN (*expr_p), pre_p, NULL, + is_gimple_call_addr); if (t == GS_ERROR) ret = GS_ERROR; - else if (is_gimple_variable (CALL_EXPR_FN (*expr_p)) - && TREE_CODE (CALL_EXPR_FN (*expr_p)) != SSA_NAME) - CALL_EXPR_FN (*expr_p) - = get_initialized_tmp_var (CALL_EXPR_FN (*expr_p), pre_p, - NULL); } if (!CALL_EXPR_FN (*expr_p)) /* Internal function call. */; --- gcc/testsuite/g++.dg/cpp1z/eval-order6.C.jj 2019-10-09 10:32:44.935416328 +0200 +++ gcc/testsuite/g++.dg/cpp1z/eval-order6.C 2019-10-09 10:32:44.935416328 +0200 @@ -0,0 +1,20 @@ +// PR c++/91987 +// { dg-do run } +// { dg-options "-fstrong-eval-order" } + +int +foo () +{ + int x = 5; + int r = x << (x = 3, 2); + if (x != 3) + __builtin_abort (); + return r; +} + +int +main () +{ + if (foo () != (5 << 2)) + __builtin_abort (); +} --- gcc/testsuite/g++.dg/cpp1z/eval-order7.C.jj 2019-10-09 10:32:44.934416343 +0200 +++ gcc/testsuite/g++.dg/cpp1z/eval-order7.C 2019-10-09 10:32:44.934416343 +0200 @@ -0,0 +1,23 @@ +// PR c++/91987 +// { dg-do run } +// { dg-options "-fstrong-eval-order" } + +int a[4] = { 1, 2, 3, 4 }; +int b[4] = { 5, 6, 7, 8 }; + +int +foo () +{ + int *x = a; + int r = x[(x = b, 3)]; + if (x != b) + __builtin_abort (); + return r; +} + +int +main () +{ + if (foo () != 4) + __builtin_abort (); +} --- gcc/testsuite/g++.dg/cpp1z/eval-order8.C.jj 2019-10-09 10:32:44.935416328 +0200 +++ gcc/testsuite/g++.dg/cpp1z/eval-order8.C 2019-10-09 10:32:44.935416328 +0200 @@ -0,0 +1,20 @@ +// PR c++/91987 +// { dg-do run } +// { dg-options "-fstrong-eval-order" } + +int a[4] = { 1, 2, 3, 4 }; +int b; + +int +main () +{ + int *x = a; + b = 1; + int r = (b = 4, x)[(b *= 2, 3)]; + if (b != 8 || r != 4) + __builtin_abort (); + b = 1; + r = (b = 3, 2)[(b *= 2, x)]; + if (b != 6 || r != 3) + __builtin_abort (); +} --- gcc/testsuite/c-c++-common/gomp/pr91987.c.jj 2019-10-09 12:42:40.059990464 +0200 +++ gcc/testsuite/c-c++-common/gomp/pr91987.c 2019-10-09 12:42:51.330820905 +0200 @@ -0,0 +1,26 @@ +/* PR c++/91987 */ + +int bar (void); +void baz (int *); +#pragma omp declare target to (baz) + +void +foo (int *a, int (*b)[10][10]) +{ + #pragma omp target map(a[bar ()]) + baz (a); + #pragma omp target map(a[bar ():1]) + baz (a); + #pragma omp target map(a[10:bar ()]) + baz (a); + #pragma omp task depend(inout:a[10:bar ()]) + baz (a); + #pragma omp task depend(inout:a[10:bar ()]) + baz (a); + #pragma omp parallel reduction(+:a[bar ():2]) + baz (a); + #pragma omp parallel reduction(+:a[2:bar ()]) + baz (a); + #pragma omp parallel reduction(+:b[bar ():2][bar ():10][bar ():10]) + baz (a); +}