From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2673 invoked by alias); 26 Feb 2018 14:37:43 -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 2661 invoked by uid 89); 26 Feb 2018 14:37:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f180.google.com Received: from mail-wr0-f180.google.com (HELO mail-wr0-f180.google.com) (209.85.128.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Feb 2018 14:37:40 +0000 Received: by mail-wr0-f180.google.com with SMTP id n7so21465806wrn.5 for ; Mon, 26 Feb 2018 06:37:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:mail-followup-to:cc:subject:references :date:in-reply-to:message-id:user-agent:mime-version; bh=ckqJwqZDNIfYETlt+sjCMyYhOoKUKmADSPQ8fXgPpq0=; b=WUHlAGzlTmLFkfkuJQbVhsDbbjJgKYXhBsLREP+Ps8wxBTHWoSEKjhVI5fqdtVulC1 lYegzS/xWG23n5YSw7R6KS9UaEpovDVg2OXxpKvo60f0VT9bMhkj/LEJx3vwYo57toqI BSn6g/xyaMWGL5X6e2tlm40m+xhXZQJRipvtRVYZQsk+QciDm3IFPNxBK/WhZ/pkIVdg LRG/XIACLIJvlZ0YXfkBxzTOJeOHw1x2e8N03fHld6dncNDKIZ/cHGlHBnAObJ+R4WiH ecVGJAq7he8WjyoWoqNPSxlFrtbJCd4czh9Rtg25fC5KBhg6nMPMOjDPfNKTyvclWyja px+Q== X-Gm-Message-State: APf1xPDIeZ04TQZdCbpxool/KV2TYotxT5bdVef3eMhVlN/feP7dnH5f Z1Nlg/YCAxo0j3LB1P8gUzIGgtMpqVM= X-Google-Smtp-Source: AH8x225ccNBlY8o2VOjIF7lvvm1Tz5LnYLfZoODQhKlA6SNbxQ+8AsGaJkGEuI7+AzQZp/BWH/IwQw== X-Received: by 10.223.139.68 with SMTP id v4mr10035702wra.23.1519655858158; Mon, 26 Feb 2018 06:37:38 -0800 (PST) Received: from localhost ([217.140.96.141]) by smtp.gmail.com with ESMTPSA id c1sm9193781wrf.31.2018.02.26.06.37.37 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 26 Feb 2018 06:37:37 -0800 (PST) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener ,GCC Patches , richard.sandiford@linaro.org Cc: GCC Patches Subject: Re: Make fix for PR 83965 handle SLP reduction chains References: <87606rsivf.fsf@linaro.org> Date: Mon, 26 Feb 2018 14:37:00 -0000 In-Reply-To: (Richard Biener's message of "Mon, 26 Feb 2018 13:22:43 +0100") Message-ID: <87371nizpr.fsf@linaro.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-02/txt/msg01426.txt.bz2 Richard Biener writes: > On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford > wrote: >> This patch prevents pattern-matching of fold-left SLP reduction chains, >> which the previous patch for 83965 didn't handle properly. It only >> stops the last statement in the group from being matched, but that's >> enough to cause the group to be dissolved later. >> >> A better fix would be to put all the information about the reduction >> on the the first statement in the reduction chain, so that every >> statement in the group can tell what the group is doing. That doesn't >> seem like stage 4 material though. >> >> As it stands, things seem to be a bit of a mess. In >> vect_force_simple_reduction we attach the reduction type and >> phi pointer to the last statement in a reduction chain: >> >> reduc_def_info = vinfo_for_stmt (def); >> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type; >> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi; >> >> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1: >> >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) = >> vect_reduction_def; >> >> This code in vectorizable_reduction gave the impression that >> everything really is keyed off the last statement: >> >> /* In case of reduction chain we switch to the first stmt in the chain, but >> we don't update STMT_INFO, since only the last stmt is marked as reduction >> and has reduction properties. */ >> if (GROUP_FIRST_ELEMENT (stmt_info) >> && GROUP_FIRST_ELEMENT (stmt_info) != stmt) >> { >> stmt = GROUP_FIRST_ELEMENT (stmt_info); >> first_p = false; >> } >> >> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull >> for SLP reduction chains, since we dissolve the group if SLP fails. >> And SLP only analyses the first statement in the group, not the last: >> >> stmt = SLP_TREE_SCALAR_STMTS (node)[0]; >> stmt_vec_info stmt_info = vinfo_for_stmt (stmt); >> [...] >> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance); >> >> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF >> are being attached to the wrong statement, since we only analyse >> the first one. But it turns out that REDUC_TYPE and REDUC_DEF >> don't matter for the first statement in the group, since that >> takes the phi as an input, and when the phi is a direct input, >> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's >> own info. The DEF_TYPE problem is handled by: >> >> /* Mark the first element of the reduction chain as reduction to properly >> transform the node. In the reduction analysis phase only the last >> element of the chain is marked as reduction. */ >> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt))) >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def; >> >> in vect_analyze_slp_instance (cancelled by: >> >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element)) >> = vect_internal_def; >> >> in vect_analyze_slp on failure), with the operation being repeated >> in vect_schedule_slp_instance (redundantly AFAICT): >> >> /* Mark the first element of the reduction chain as reduction to properly >> transform the node. In the analysis phase only the last element of the >> chain is marked as reduction. */ >> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS (stmt_info) >> && GROUP_FIRST_ELEMENT (stmt_info) == stmt) >> { >> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def; >> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type; >> } >> >> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. >> OK to install? > > Ok for stage1. It's a GCC 8 regression, so OK for stage4? Richard > Richard. > >> Richard >> >> >> 2018-02-20 Richard Sandiford >> >> gcc/ >> PR tree-optimization/83965 >> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume >> that grouped statements are part of a reduction chain. Return >> true if the statement is not marked as a reduction itself but >> is part of a group. >> (vect_recog_dot_prod_pattern): Don't check whether the statement >> is part of a group here. >> (vect_recog_sad_pattern): Likewise. >> (vect_recog_widen_sum_pattern): Likewise. >> >> gcc/testsuite/ >> PR tree-optimization/83965 >> * gcc.dg/vect/pr83965-2.c: New test. >> >> Index: gcc/tree-vect-patterns.c >> =================================================================== >> --- gcc/tree-vect-patterns.c 2018-02-20 09:40:41.843451227 +0000 >> +++ gcc/tree-vect-patterns.c 2018-02-20 16:28:55.636762056 +0000 >> @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp >> } >> >> /* Return true if STMT_VINFO describes a reduction for which reassociation >> - is allowed. */ >> + is allowed. If STMT_INFO is part of a group, assume that it's part of >> + a reduction chain and optimistically assume that all statements >> + except the last allow reassociation. */ >> >> static bool >> vect_reassociating_reduction_p (stmt_vec_info stmt_vinfo) >> { >> return (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def >> - && STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION); >> + ? STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION >> + : GROUP_FIRST_ELEMENT (stmt_vinfo) != NULL); >> } >> >> /* Function vect_recog_dot_prod_pattern >> @@ -350,8 +353,7 @@ vect_recog_dot_prod_pattern (vec> { >> gimple *def_stmt; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> oprnd0 = gimple_assign_rhs1 (last_stmt); >> oprnd1 = gimple_assign_rhs2 (last_stmt); >> @@ -571,8 +573,7 @@ vect_recog_sad_pattern (vec *s >> { >> gimple *def_stmt; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> plus_oprnd0 = gimple_assign_rhs1 (last_stmt); >> plus_oprnd1 = gimple_assign_rhs2 (last_stmt); >> @@ -1256,8 +1257,7 @@ vect_recog_widen_sum_pattern (vec> if (gimple_assign_rhs_code (last_stmt) != PLUS_EXPR) >> return NULL; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> >> oprnd0 = gimple_assign_rhs1 (last_stmt); >> Index: gcc/testsuite/gcc.dg/vect/pr83965-2.c >> =================================================================== >> --- /dev/null 2018-02-19 19:34:42.906488063 +0000 >> +++ gcc/testsuite/gcc.dg/vect/pr83965-2.c 2018-02-20 16:28:55.635762095 +0000 >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile } */ >> +/* { dg-additional-options "-Ofast -ftrapv" } */ >> + >> +int c; >> +unsigned char d; >> +int e (unsigned char *f) >> +{ >> + int g; >> + for (int a; a; a++) >> + { >> + for (int b = 0; b < 6; b++) >> + g += __builtin_abs (f[b] - d); >> + f += c; >> + } >> + return g; >> +}