From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76743 invoked by alias); 2 Nov 2016 11:32:07 -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 76672 invoked by uid 89); 2 Nov 2016 11:32:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=wi, 2016-11-02 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 02 Nov 2016 11:31:46 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9CB44ACF3; Wed, 2 Nov 2016 11:31:44 +0000 (UTC) Date: Wed, 02 Nov 2016 11:32:00 -0000 From: Richard Biener To: gcc-patches@gcc.gnu.org cc: kyrylo.tkachov@arm.com Subject: Re: [PATCH] Fix store-merging alias check, apply some TLC In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2016-11/txt/msg00153.txt.bz2 On Wed, 2 Nov 2016, Richard Biener wrote: > > This fixes the alias check in terminate_all_aliasing_chains -- the > base we use for the hash table indexing does not constitute an object > that aliases all stores in the chain (consider for example the MEM_REF > handling which strips the offset completely). I fat-fingered this thinking one step ahead (forgot to take the address). > I've refactored data structures a bit in the process of making > (as a followup) 'base_addr' a true address (and thus avoid building > that new MEM_REF for example). A followup will then try to support > (some) bases with offset != NULL_TREE. > > Bootstrap / regtest running on x86_64-unknown-linux-gnu. So I had to re-start the testing when I already finished the very minor change to make addr_base actually the address. Thus the 2nd part is now included. This also has the advantage that the hash table comparison will now apply the lax rules valid for comparing addresses (thus the TREE_THIS_VOLATILE check should be no longer needed for example and also different TBAA stores will now be merged). I took the liberty to explicitely rule out TARGET_MEM_REFs from the merging (they'll have failed to form groups anyway as they are not subsetted and we don't handle the const offset stripping yet to eventually merge byte to word stores). Richard. 2016-11-02 Richard Biener * gimple-ssa-store-merging.c (struct store_immediate_info): Remove redundant val and dest members. (store_immediate_info::store_immediate_info): Adjust. (merged_store_group::merged_store_group): Adjust. (merged_store_group::apply_stores): Likewise. (struct imm_store_chain_info): Add base_addr field. (imm_store_chain_info::imm_store_chain_info): New constructor. (imm_store_chain_info::terminate_and_process_chain): Do not pass base. (imm_store_chain_info::output_merged_store): Likewise. Use addr_base which is already the address. (imm_store_chain_info::output_merged_stores): Likewise. (pass_tree_store_merging::terminate_all_aliasing_chains): Take imm_store_chain_info instead of base. Fix alias check. (pass_tree_store_merging::terminate_and_release_chain): Likewise. (imm_store_chain_info::coalesce_immediate_stores): Adjust. (pass_store_merging::execute): Refuse to operate on TARGET_MEM_REF. use the address of the base and adjust for other changes. Index: gcc/gimple-ssa-store-merging.c =================================================================== --- gcc/gimple-ssa-store-merging.c (revision 241779) +++ gcc/gimple-ssa-store-merging.c (working copy) @@ -140,19 +140,17 @@ struct store_immediate_info { unsigned HOST_WIDE_INT bitsize; unsigned HOST_WIDE_INT bitpos; - tree val; - tree dest; gimple *stmt; unsigned int order; - store_immediate_info (unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, tree, - tree, gimple *, unsigned int); + store_immediate_info (unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, + gimple *, unsigned int); }; store_immediate_info::store_immediate_info (unsigned HOST_WIDE_INT bs, - unsigned HOST_WIDE_INT bp, tree v, - tree d, gimple *st, + unsigned HOST_WIDE_INT bp, + gimple *st, unsigned int ord) - : bitsize (bs), bitpos (bp), val (v), dest (d), stmt (st), order (ord) + : bitsize (bs), bitpos (bp), stmt (st), order (ord) { } @@ -557,7 +555,7 @@ merged_store_group::merged_store_group ( /* VAL has memory allocated for it in apply_stores once the group width has been finalized. */ val = NULL; - align = get_object_alignment (info->dest); + align = get_object_alignment (gimple_assign_lhs (info->stmt)); stores.create (1); stores.safe_push (info); last_stmt = info->stmt; @@ -654,14 +652,16 @@ merged_store_group::apply_stores () FOR_EACH_VEC_ELT (stores, i, info) { unsigned int pos_in_buffer = info->bitpos - start; - bool ret = encode_tree_to_bitpos (info->val, val, info->bitsize, - pos_in_buffer, buf_size); + bool ret = encode_tree_to_bitpos (gimple_assign_rhs1 (info->stmt), + val, info->bitsize, + pos_in_buffer, buf_size); if (dump_file && (dump_flags & TDF_DETAILS)) { if (ret) { fprintf (dump_file, "After writing "); - print_generic_expr (dump_file, info->val, 0); + print_generic_expr (dump_file, + gimple_assign_rhs1 (info->stmt), 0); fprintf (dump_file, " of size " HOST_WIDE_INT_PRINT_DEC " at position %d the merged region contains:\n", info->bitsize, pos_in_buffer); @@ -680,13 +680,15 @@ merged_store_group::apply_stores () struct imm_store_chain_info { + tree base_addr; auto_vec m_store_info; auto_vec m_merged_store_groups; - bool terminate_and_process_chain (tree); + imm_store_chain_info (tree b_a) : base_addr (b_a) {} + bool terminate_and_process_chain (); bool coalesce_immediate_stores (); - bool output_merged_store (tree, merged_store_group *); - bool output_merged_stores (tree); + bool output_merged_store (merged_store_group *); + bool output_merged_stores (); }; const pass_data pass_data_tree_store_merging = { @@ -722,8 +724,9 @@ private: hash_map m_stores; bool terminate_and_process_all_chains (); - bool terminate_all_aliasing_chains (tree, tree, bool, gimple *); - bool terminate_and_release_chain (tree); + bool terminate_all_aliasing_chains (tree, imm_store_chain_info **, + bool, gimple *); + bool terminate_and_release_chain (imm_store_chain_info *); }; // class pass_store_merging /* Terminate and process all recorded chains. Return true if any changes @@ -736,7 +739,7 @@ pass_store_merging::terminate_and_proces = m_stores.begin (); bool ret = false; for (; iter != m_stores.end (); ++iter) - ret |= terminate_and_release_chain ((*iter).first); + ret |= terminate_and_release_chain ((*iter).second); return ret; } @@ -750,7 +753,9 @@ pass_store_merging::terminate_and_proces If that is the case we have to terminate any chain anchored at BASE. */ bool -pass_store_merging::terminate_all_aliasing_chains (tree dest, tree base, +pass_store_merging::terminate_all_aliasing_chains (tree dest, + imm_store_chain_info + **chain_info, bool var_offset_p, gimple *stmt) { @@ -760,44 +765,38 @@ pass_store_merging::terminate_all_aliasi if (!gimple_vuse (stmt)) return false; - struct imm_store_chain_info **chain_info = NULL; - /* Check if the assignment destination (BASE) is part of a store chain. This is to catch non-constant stores to destinations that may be part of a chain. */ - if (base) + if (chain_info) { - chain_info = m_stores.get (base); - if (chain_info) + /* We have a chain at BASE and we're writing to [BASE + ]. + This can interfere with any of the stores so terminate + the chain. */ + if (var_offset_p) { - /* We have a chain at BASE and we're writing to [BASE + ]. - This can interfere with any of the stores so terminate - the chain. */ - if (var_offset_p) - { - terminate_and_release_chain (base); - ret = true; - } - /* Otherwise go through every store in the chain to see if it - aliases with any of them. */ - else + terminate_and_release_chain (*chain_info); + ret = true; + } + /* Otherwise go through every store in the chain to see if it + aliases with any of them. */ + else + { + struct store_immediate_info *info; + unsigned int i; + FOR_EACH_VEC_ELT ((*chain_info)->m_store_info, i, info) { - struct store_immediate_info *info; - unsigned int i; - FOR_EACH_VEC_ELT ((*chain_info)->m_store_info, i, info) + if (stmt_may_clobber_ref_p (info->stmt, dest)) { - if (refs_may_alias_p (info->dest, dest)) + if (dump_file && (dump_flags & TDF_DETAILS)) { - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, - "stmt causes chain termination:\n"); - print_gimple_stmt (dump_file, stmt, 0, 0); - } - terminate_and_release_chain (base); - ret = true; - break; + fprintf (dump_file, + "stmt causes chain termination:\n"); + print_gimple_stmt (dump_file, stmt, 0, 0); } + terminate_and_release_chain (*chain_info); + ret = true; + break; } } } @@ -814,11 +813,16 @@ pass_store_merging::terminate_all_aliasi if (chain_info && (*chain_info) == (*iter).second) continue; - tree key = (*iter).first; - if (ref_maybe_used_by_stmt_p (stmt, key) - || stmt_may_clobber_ref_p (stmt, key)) + /* We can't use the base object here as that does not reliably exist. + Build a ao_ref from the base object address (if we know the + minimum and maximum offset and the maximum size we could improve + things here). */ + ao_ref chain_ref; + ao_ref_init_from_ptr_and_size (&chain_ref, (*iter).first, NULL_TREE); + if (ref_maybe_used_by_stmt_p (stmt, &chain_ref) + || stmt_may_clobber_ref_p_1 (stmt, &chain_ref)) { - terminate_and_release_chain (key); + terminate_and_release_chain ((*iter).second); ret = true; } } @@ -831,19 +835,11 @@ pass_store_merging::terminate_all_aliasi entry is removed after the processing in any case. */ bool -pass_store_merging::terminate_and_release_chain (tree base) +pass_store_merging::terminate_and_release_chain (imm_store_chain_info *chain_info) { - struct imm_store_chain_info **chain_info = m_stores.get (base); - - if (!chain_info) - return false; - - gcc_assert (*chain_info); - - bool ret = (*chain_info)->terminate_and_process_chain (base); - delete *chain_info; - m_stores.remove (base); - + bool ret = chain_info->terminate_and_process_chain (); + m_stores.remove (chain_info->base_addr); + delete chain_info; return ret; } @@ -880,7 +876,7 @@ imm_store_chain_info::coalesce_immediate fprintf (dump_file, "Store %u:\nbitsize:" HOST_WIDE_INT_PRINT_DEC " bitpos:" HOST_WIDE_INT_PRINT_DEC " val:\n", i, info->bitsize, info->bitpos); - print_generic_expr (dump_file, info->val, 0); + print_generic_expr (dump_file, gimple_assign_rhs1 (info->stmt), 0); fprintf (dump_file, "\n------------\n"); } @@ -1103,7 +1099,7 @@ split_group (merged_store_group *group, return true. */ bool -imm_store_chain_info::output_merged_store (tree base, merged_store_group *group) +imm_store_chain_info::output_merged_store (merged_store_group *group) { unsigned HOST_WIDE_INT start_byte_pos = group->start / BITS_PER_UNIT; @@ -1141,8 +1137,7 @@ imm_store_chain_info::output_merged_stor tree int_type = build_nonstandard_integer_type (try_size, UNSIGNED); int_type = build_aligned_type (int_type, align); - tree addr = build_fold_addr_expr (base); - tree dest = fold_build2 (MEM_REF, int_type, addr, + tree dest = fold_build2 (MEM_REF, int_type, base_addr, build_int_cst (offset_type, try_pos)); tree src = native_interpret_expr (int_type, @@ -1213,14 +1208,14 @@ imm_store_chain_info::output_merged_stor successful. Return true iff any changes were made. */ bool -imm_store_chain_info::output_merged_stores (tree base) +imm_store_chain_info::output_merged_stores () { unsigned int i; merged_store_group *merged_store; bool ret = false; FOR_EACH_VEC_ELT (m_merged_store_groups, i, merged_store) { - if (output_merged_store (base, merged_store)) + if (output_merged_store (merged_store)) { unsigned int j; store_immediate_info *store; @@ -1250,7 +1245,7 @@ imm_store_chain_info::output_merged_stor Return true if any changes were made. */ bool -imm_store_chain_info::terminate_and_process_chain (tree base) +imm_store_chain_info::terminate_and_process_chain () { /* Process store chain. */ bool ret = false; @@ -1258,7 +1253,7 @@ imm_store_chain_info::terminate_and_proc { ret = coalesce_immediate_stores (); if (ret) - ret = output_merged_stores (base); + ret = output_merged_stores (); } /* Delete all the entries we allocated ourselves. */ @@ -1381,28 +1376,29 @@ pass_store_merging::execute (function *f other so we can't track chains on them. */ || TREE_THIS_VOLATILE (base_addr); + /* We do not want to rewrite TARGET_MEM_REFs. */ + if (TREE_CODE (base_addr) == TARGET_MEM_REF) + invalid = true; /* In some cases get_inner_reference may return a MEM_REF [ptr + byteoffset]. For the purposes of this pass canonicalize the base_addr to MEM_REF [ptr] and take byteoffset into account in the bitpos. This occurs in PR 23684 and this way we can catch more chains. */ - if (TREE_CODE (base_addr) == MEM_REF - && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (base_addr, 0)))) + else if (TREE_CODE (base_addr) == MEM_REF) { offset_int bit_off, byte_off = mem_ref_offset (base_addr); bit_off = byte_off << LOG2_BITS_PER_UNIT; bit_off += bitpos; if (!wi::neg_p (bit_off) && wi::fits_shwi_p (bit_off)) - { - bitpos = bit_off.to_shwi (); - base_addr = build2 (MEM_REF, TREE_TYPE (base_addr), - TREE_OPERAND (base_addr, 0), - build_zero_cst (TREE_TYPE ( - TREE_OPERAND (base_addr, 1)))); - } + bitpos = bit_off.to_shwi (); else invalid = true; + base_addr = TREE_OPERAND (base_addr, 0); } + /* get_inner_reference returns the base object, get at its + address now. */ + else + base_addr = build_fold_addr_expr (base_addr); struct imm_store_chain_info **chain_info = m_stores.get (base_addr); @@ -1413,7 +1409,7 @@ pass_store_merging::execute (function *f if (chain_info) { info = new store_immediate_info ( - bitsize, bitpos, rhs, lhs, stmt, + bitsize, bitpos, stmt, (*chain_info)->m_store_info.length ()); if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -1432,17 +1428,17 @@ pass_store_merging::execute (function *f fprintf (dump_file, "Reached maximum number of statements" " to merge:\n"); - terminate_and_release_chain (base_addr); + terminate_and_release_chain (*chain_info); } continue; } /* Store aliases any existing chain? */ - terminate_all_aliasing_chains (lhs, base_addr, false, stmt); + terminate_all_aliasing_chains (lhs, chain_info, false, stmt); /* Start a new chain. */ struct imm_store_chain_info *new_chain - = new imm_store_chain_info; - info = new store_immediate_info (bitsize, bitpos, rhs, lhs, + = new imm_store_chain_info (base_addr); + info = new store_immediate_info (bitsize, bitpos, stmt, 0); new_chain->m_store_info.safe_push (info); m_stores.put (base_addr, new_chain); @@ -1457,13 +1453,13 @@ pass_store_merging::execute (function *f } } else - terminate_all_aliasing_chains (lhs, base_addr, + terminate_all_aliasing_chains (lhs, chain_info, offset != NULL_TREE, stmt); continue; } - terminate_all_aliasing_chains (NULL_TREE, NULL_TREE, false, stmt); + terminate_all_aliasing_chains (NULL_TREE, NULL, false, stmt); } terminate_and_process_all_chains (); }