From patchwork Mon Feb 1 22:07:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikael Morin X-Patchwork-Id: 576712 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 DDC6A140BA1 for ; Tue, 2 Feb 2016 09:08:02 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=YN5t8hoy; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=ICoVUBNrLQlFkSJZqPlWuSEUIbO9YLZb4LKB/4tLuDkyY05iV4VFT 4Z4FyFE7dH7LUFpyScG0Il2B0pzOOomuzgaa9j3pZEzKIK1LAxvv3Iyu6Jw+L3Op uvOJBt+sCckwAdRjql2aRChHG7mQJz2EZAQf1OkitihhdxM6EbMzC0= 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:from :subject:to:message-id:date:mime-version:content-type; s= default; bh=dWbWxgLNUqrbqGHMMSyZy3jQhv8=; b=YN5t8hoyZpP1bcgi6zDU u5n8Gf7w3Ai04EnfTiQPqn1poBc5Lj0g1vSBi+eWwScS92i+Q+3fq4gsdEjQz0bG 2n+NVBQHZRRWiOwHg4WnuafTayS65rFbQIb69C3yFX098KYVCe92T7iTBOqKj1gR sm5E33Q2OZ5IF2KaE0LE4IU= Received: (qmail 18993 invoked by alias); 1 Feb 2016 22:07:51 -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 18972 invoked by uid 89); 1 Feb 2016 22:07:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=no version=3.3.2 spammy=H*F:D*sfr.fr, intermediary, 44946, sk:array_o X-Spam-User: qpsmtpd, 2 recipients X-HELO: smtp22.services.sfr.fr Received: from smtp22.services.sfr.fr (HELO smtp22.services.sfr.fr) (93.17.128.13) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 01 Feb 2016 22:07:48 +0000 Received: from filter.sfr.fr (localhost [46.193.167.111]) by msfrf2214.sfr.fr (SMTP Server) with ESMTP id 0F43770000BB; Mon, 1 Feb 2016 23:07:45 +0100 (CET) Authentication-Results: sfrmc.priv.atos.fr; dkim=none (no signature); dkim-adsp=none (no policy) header.from=mikael.morin@sfr.fr Received: from [10.188.164.151] (ppp-seco21th2-46-193-167-111.wb.wifirst.net [46.193.167.111]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by msfrf2214.sfr.fr (SMTP Server) with ESMTP id 6C6CF70000B2; Mon, 1 Feb 2016 23:07:44 +0100 (CET) X-SFR-UUID: 20160201220744444.6C6CF70000B2@msfrf2214.sfr.fr From: Mikael Morin Subject: [Patch, fortran] PR66089 fix elemental dependency mishandling To: gfortran , gcc-patches Message-ID: <56AFD723.2050709@sfr.fr> Date: Mon, 1 Feb 2016 23:07:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 X-IsSubscribed: yes Hello, this is about the case c(:) = elemental_func(c(1), ...) where as a result of a trunk change, only a reference to c(1) is saved to a temporary variable, instead of its value. The fix tries to save the amount of copying as much as possible by detecting the above case. Technically through the usage of a new field needs_temporary. The patch is a variant of the one that has been on bugzilla for months. The main difference is the usage of gfc_expr_is_variable instead of the check for expr_type == EXPR_VARIABLE (the former includes pointer-returning functions as well). Regression-tested on x86_64-unknown-linux-gnu. OK for trunk? Mikael 2016-02-01 Mikael Morin PR fortran/66089 * trans-expr.c (expr_is_variable, gfc_expr_is_variable): Rename the former to the latter and make it non-static. Update callers. * gfortran.h (gfc_expr_is_variable): New declaration. (struct gfc_ss_info): Add field needs_temporary. * trans-array.c (gfc_scalar_elemental_arg_saved_as_argument): Tighten the condition on aggregate expressions with a check that the expression is a variable and doesn't need a temporary. (gfc_conv_resolve_dependency): Add intermediary reference variable. Set the needs_temporary field. 2016-02-01 Mikael Morin PR fortran/66089 * gfortran.dg/elemental_dependency_6.f90: New. diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c index eeb688c..2ff2833 100644 --- a/gcc/fortran/trans-array.c +++ b/gcc/fortran/trans-array.c @@ -2464,10 +2464,12 @@ gfc_scalar_elemental_arg_saved_as_reference (gfc_ss_info * ss_info) return true; /* If the expression is a data reference of aggregate type, + and the data reference is not used on the left hand side, avoid a copy by saving a reference to the content. */ - if (ss_info->expr->expr_type == EXPR_VARIABLE + if (!ss_info->data.scalar.needs_temporary && (ss_info->expr->ts.type == BT_DERIVED - || ss_info->expr->ts.type == BT_CLASS)) + || ss_info->expr->ts.type == BT_CLASS) + && gfc_expr_is_variable (ss_info->expr)) return true; /* Otherwise the expression is evaluated to a temporary variable before the @@ -4461,6 +4463,7 @@ gfc_conv_resolve_dependencies (gfc_loopinfo * loop, gfc_ss * dest, gfc_ss *ss; gfc_ref *lref; gfc_ref *rref; + gfc_ss_info *ss_info; gfc_expr *dest_expr; gfc_expr *ss_expr; int nDepend = 0; @@ -4471,15 +4474,16 @@ gfc_conv_resolve_dependencies (gfc_loopinfo * loop, gfc_ss * dest, for (ss = rss; ss != gfc_ss_terminator; ss = ss->next) { - ss_expr = ss->info->expr; + ss_info = ss->info; + ss_expr = ss_info->expr; - if (ss->info->array_outer_dependency) + if (ss_info->array_outer_dependency) { nDepend = 1; break; } - if (ss->info->type != GFC_SS_SECTION) + if (ss_info->type != GFC_SS_SECTION) { if (flag_realloc_lhs && dest_expr != ss_expr @@ -4494,6 +4498,10 @@ gfc_conv_resolve_dependencies (gfc_loopinfo * loop, gfc_ss * dest, nDepend = gfc_check_dependency (dest_expr, ss_expr, false); + if (ss_info->type == GFC_SS_REFERENCE + && gfc_check_dependency (dest_expr, ss_expr, false)) + ss_info->data.scalar.needs_temporary = 1; + continue; } diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c index c5ae4c5..9f5bece 100644 --- a/gcc/fortran/trans-expr.c +++ b/gcc/fortran/trans-expr.c @@ -8825,8 +8825,8 @@ gfc_trans_array_constructor_copy (gfc_expr * expr1, gfc_expr * expr2) /* Tells whether the expression is to be treated as a variable reference. */ -static bool -expr_is_variable (gfc_expr *expr) +bool +gfc_expr_is_variable (gfc_expr *expr) { gfc_expr *arg; gfc_component *comp; @@ -8839,7 +8839,7 @@ expr_is_variable (gfc_expr *expr) if (arg) { gcc_assert (expr->value.function.isym->id == GFC_ISYM_TRANSPOSE); - return expr_is_variable (arg); + return gfc_expr_is_variable (arg); } /* A data-pointer-returning function should be considered as a variable @@ -9320,7 +9320,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag, must have its components deallocated afterwards. */ scalar_to_array = (expr2->ts.type == BT_DERIVED && expr2->ts.u.derived->attr.alloc_comp - && !expr_is_variable (expr2) + && !gfc_expr_is_variable (expr2) && expr1->rank && !expr2->rank); scalar_to_array |= (expr1->ts.type == BT_DERIVED && expr1->rank @@ -9364,7 +9364,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag, } tmp = gfc_trans_scalar_assign (&lse, &rse, expr1->ts, - expr_is_variable (expr2) || scalar_to_array + gfc_expr_is_variable (expr2) || scalar_to_array || expr2->expr_type == EXPR_ARRAY, !(l_is_temp || init_flag) && dealloc); gfc_add_expr_to_block (&body, tmp); diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h index 3026e3b..316ee9b 100644 --- a/gcc/fortran/trans.h +++ b/gcc/fortran/trans.h @@ -210,6 +210,10 @@ typedef struct gfc_ss_info this is the symbol of the corresponding dummy argument. */ gfc_symbol *dummy_arg; tree value; + /* Tells that the scalar is a reference to a variable that might + be present on the lhs, so that we should evaluate the value + itself before the loop, not just the reference. */ + unsigned needs_temporary:1; } scalar; @@ -464,6 +468,7 @@ bool gfc_conv_ieee_arithmetic_function (gfc_se *, gfc_expr *); tree gfc_save_fp_state (stmtblock_t *); void gfc_restore_fp_state (stmtblock_t *, tree); +bool gfc_expr_is_variable (gfc_expr *); /* Does an intrinsic map directly to an external library call This is true for array-returning intrinsics, unless diff --git a/gcc/testsuite/gfortran.dg/elemental_dependency_6.f90 b/gcc/testsuite/gfortran.dg/elemental_dependency_6.f90 new file mode 100644 index 0000000..52753f1 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/elemental_dependency_6.f90 @@ -0,0 +1,28 @@ +! { dg-do run } +! +! PR fortran/66089 +! Check that we do create a temporary for C(1) below in the assignment +! to C. + + type :: t + integer :: c + end type t + + type(t), dimension(5) :: a, b, c + + a = t(1) + b = t(7) + c = t(13) + c = plus(c(1), b) +! print *, c + if (any(c%c /= 20)) call abort + +contains + + elemental function plus(lhs, rhs) + type(t), intent(in) :: lhs, rhs + type(t) :: plus + plus%c = lhs%c + rhs%c + end function plus + +end