From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29687 invoked by alias); 16 Jun 2016 16:54:52 -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 29668 invoked by uid 89); 16 Jun 2016 16:54:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=adhere, among 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; Thu, 16 Jun 2016 16:54:40 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A64643B72F; Thu, 16 Jun 2016 16:54:39 +0000 (UTC) Received: from localhost.localdomain (ovpn-116-111.phx2.redhat.com [10.3.116.111]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5GGsdCU007262; Thu, 16 Jun 2016 12:54:39 -0400 Subject: Re: [PATCH, vec-tails 07/10] Support loop epilogue combining To: Ilya Enkovich , gcc-patches@gcc.gnu.org References: <20160519194450.GH40563@msticlxl57.ims.intel.com> From: Jeff Law Message-ID: <18ccae1a-30c3-c23c-e28f-287f9d41eaa0@redhat.com> Date: Thu, 16 Jun 2016 16:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160519194450.GH40563@msticlxl57.ims.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg01266.txt.bz2 On 05/19/2016 01:44 PM, Ilya Enkovich wrote: > Hi, > > This patch introduces support for loop epilogue combining. This includes > support in cost estimation and all required changes required to mask > vectorized loop. > > Thanks, > Ilya > -- > gcc/ > > 2016-05-19 Ilya Enkovich > > * dbgcnt.def (vect_tail_combine): New. > * params.def (PARAM_VECT_COST_INCREASE_COMBINE_THRESHOLD): New. > * tree-vect-data-refs.c (vect_get_new_ssa_name): Support vect_mask_var. > * tree-vect-loop-manip.c (slpeel_tree_peel_loop_to_edge): Support > epilogue combined with loop body. > (vect_do_peeling_for_loop_bound): Likewise. > * tree-vect-loop.c Include alias.h and dbgcnt.h. > (vect_estimate_min_profitable_iters): Add ret_min_profitable_combine_niters > arg, compute number of iterations for which loop epilogue combining is > profitable. > (vect_generate_tmps_on_preheader): Support combined apilogue. > (vect_gen_ivs_for_masking): New. > (vect_get_mask_index_for_elems): New. > (vect_get_mask_index_for_type): New. > (vect_gen_loop_masks): New. > (vect_mask_reduction_stmt): New. > (vect_mask_mask_load_store_stmt): New. > (vect_mask_load_store_stmt): New. > (vect_combine_loop_epilogue): New. > (vect_transform_loop): Support combined apilogue. > > > diff --git a/gcc/tree-vect-loop-manip.c b/gcc/tree-vect-loop-manip.c > index fab5879..b3c0668 100644 > --- a/gcc/tree-vect-loop-manip.c > +++ b/gcc/tree-vect-loop-manip.c > @@ -1464,11 +1469,20 @@ slpeel_tree_peel_loop_to_edge (struct loop *loop, struct loop *scalar_loop, > bb_between_loops = new_exit_bb; > bb_after_second_loop = split_edge (single_exit (second_loop)); > > - pre_condition = > - fold_build2 (EQ_EXPR, boolean_type_node, *first_niters, niters); > - skip_e = slpeel_add_loop_guard (bb_between_loops, pre_condition, NULL, > - bb_after_second_loop, bb_before_first_loop, > - inverse_probability (second_guard_probability)); > + if (skip_second_after_first) > + /* We can just redirect edge from bb_between_loops to > + bb_after_second_loop but we have many code assuming > + we have a guard after the first loop. So just make > + always taken condtion. */ > + pre_condition = fold_build2 (EQ_EXPR, boolean_type_node, integer_zero_node, > + integer_zero_node); This isn't ideal, but I don't think it's that big of an issue. > @@ -1758,8 +1772,10 @@ vect_do_peeling_for_loop_bound (loop_vec_info loop_vinfo, > basic_block preheader; > int loop_num; > int max_iter; > + int bound2; > tree cond_expr = NULL_TREE; > gimple_seq cond_expr_stmt_list = NULL; > + bool combine = LOOP_VINFO_COMBINE_EPILOGUE (loop_vinfo); > > if (dump_enabled_p ()) > dump_printf_loc (MSG_NOTE, vect_location, > @@ -1769,12 +1785,13 @@ vect_do_peeling_for_loop_bound (loop_vec_info loop_vinfo, > > loop_num = loop->num; > > + bound2 = combine ? th : LOOP_VINFO_VECT_FACTOR (loop_vinfo); Can you document what the TH parameter is to the various routines that use it in tree-vect-loop-manip.c? I realize you didn't add it, but it would help anyone looking at this code in the future to know it's the threshold of iterations for vectorization without having to find it in other function comment headers ;-) That's pre-approved to go in immediately :-) > @@ -1803,7 +1820,11 @@ vect_do_peeling_for_loop_bound (loop_vec_info loop_vinfo, > max_iter = (LOOP_VINFO_PEELING_FOR_GAPS (loop_vinfo) > ? LOOP_VINFO_VECT_FACTOR (loop_vinfo) * 2 > : LOOP_VINFO_VECT_FACTOR (loop_vinfo)) - 2; > - if (check_profitability) > + /* When epilogue is combined only profitability > + treshold matters. */ s/treshold/threshold/ > static void > vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo, > int *ret_min_profitable_niters, > - int *ret_min_profitable_estimate) > + int *ret_min_profitable_estimate, > + int *ret_min_profitable_combine_niters) I'm torn a bit here. There's all kinds of things missing/incomplete in the function comments throughout the vectorizer. And in some cases, like this one, the parameters are largely self-documenting. But we've also got coding standards that we'd like to adhere to. I don't think it's fair to require you to fix all these issues in the vectorizer (though if you wanted to, I'd fully support those an independent cleanups). Perhaps just document LOOP_VINFO with a generic comment about the ret_* parameters for this function rather than a comment for each ret_* parameter. Pre-approved for the trunk independent of the vec-tails work. > @@ -3728,6 +3784,77 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo, > min_profitable_estimate); > > + > + unsigned combine_treshold > + = PARAM_VALUE (PARAM_VECT_COST_INCREASE_COMBINE_THRESHOLD); > + /* Calculate profitability combining epilogue with the main loop. > + We have a threshold for inside cost overhead (not applied > + for low trip count loop case): > + MIC * 100 < VIC * CT > + Masked iteration should be better than a scalar prologue: > + MIC + VIC < SIC * epilogue_niters */ Can you double-check the whitespace formatting here. Where does the "100" come from and should it be a param? > @@ -6886,6 +7030,485 @@ vect_generate_tmps_on_preheader (loop_vec_info loop_vinfo, > return; > } > > + > +/* Function vect_gen_loop_masks. > + > + Create masks to mask a loop desvribed by LOOP_VINFO. Masks s/desvribed/described/ > + are created according to LOOP_VINFO_REQUIRED_MASKS and are stored > + into MASKS vector. > + > + Index of a mask in a vector is computed according to a number > + of masks's elements. Masks are sorted by number of its elements > + in descending order. Index 0 is used to access a mask with > + current_vector_size elements. Among masks with the same number > + of elements the one with lower index is used to mask iterations > + with smaller iteration counter. Note that you may get NULL elements > + for masks which are not required. Use vect_get_mask_index_for_elems > + or vect_get_mask_index_for_type to access resulting vector. */ > + > +static void > +vect_gen_loop_masks (loop_vec_info loop_vinfo, vec *masks) > +{ > + struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo); > + edge pe = loop_preheader_edge (loop); > + tree niters = LOOP_VINFO_NITERS (loop_vinfo); > + unsigned min_mask_elems, max_mask_elems, nmasks; > + unsigned iv_elems, cur_mask, prev_mask, cur_mask_elems; > + auto_vec ivs; > + tree vectype, mask_type; > + tree vec_niters, vec_niters_val, mask; > + gimple *stmt; > + basic_block bb; > + gimple_stmt_iterator gsi = gsi_after_labels (loop->header); > + unsigned vec_size; > + > + /* Create required IVs. */ > + vect_gen_ivs_for_masking (loop_vinfo, &ivs); > + vectype = TREE_TYPE (ivs[0]); > + > + vec_size = tree_to_uhwi (TYPE_SIZE_UNIT (vectype)); > + iv_elems = TYPE_VECTOR_SUBPARTS (vectype); > + > + /* Get a proper niter to build a vector. */ > + if (!is_gimple_val (niters)) > + { > + gimple_seq seq = NULL; > + niters = force_gimple_operand (niters, &seq, true, NULL); > + gsi_insert_seq_on_edge_immediate (pe, seq); > + } > + /* We may need a type cast in case niter has a too small type > + for generated IVs. */ Nit. There should be vertical whitespace after the close brace and the comment for the next logical block of code. Can you do a scan over the patchkit looking for other instances where the vertical whitespace is needed. Generally, if you find that a blob of code needs a comment, then the comment and blob of code should have that vertical whitespace to visually separate it from everything else. > +/* Function vect_combine_loop_epilogue. > + > + Combine loop epilogue with the main vectorized body. It requires > + masking of memory accesses and reductions. */ So you mask reductions, loads & stores. Is there anything else that we might potentially need to mask to combine the loop & epilogue via masking? I don't see anything particularly worrisome here either -- I have a slight concern about correctness issues with only masking loads/stores and reductions. But I will defer to your judgment on whether or not there's other stuff that we need to mask to combine the epilogue with the loop via masking. Jeff