From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83006 invoked by alias); 16 Nov 2015 09:58:34 -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 82994 invoked by uid 89); 16 Nov 2015 09:58:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f169.google.com Received: from mail-yk0-f169.google.com (HELO mail-yk0-f169.google.com) (209.85.160.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 16 Nov 2015 09:58:31 +0000 Received: by ykdr82 with SMTP id r82so229538001ykd.3 for ; Mon, 16 Nov 2015 01:58:28 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.129.133.69 with SMTP id v66mr4890190ywf.68.1447667908764; Mon, 16 Nov 2015 01:58:28 -0800 (PST) Received: by 10.37.93.11 with HTTP; Mon, 16 Nov 2015 01:58:28 -0800 (PST) In-Reply-To: References: Date: Mon, 16 Nov 2015 09:58:00 -0000 Message-ID: Subject: Re: [RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap assumptions. From: Richard Biener To: "Kumar, Venkataramanan" Cc: Bernhard Reutner-Fischer , Andrew Pinski , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg01914.txt.bz2 On Sun, Nov 15, 2015 at 12:14 PM, Kumar, Venkataramanan wrote: > Hi Richard and Bernhard. > >> -----Original Message----- >> From: Richard Biener [mailto:richard.guenther@gmail.com] >> Sent: Tuesday, November 10, 2015 5:33 PM >> To: Kumar, Venkataramanan >> Cc: Andrew Pinski; gcc-patches@gcc.gnu.org >> Subject: Re: [RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap >> assumptions. >> >> On Sat, Nov 7, 2015 at 12:41 PM, Kumar, Venkataramanan >> wrote: >> > Hi Richard, >> > >> > I have now implemented storing of DR and references using hash maps. >> > Please find attached patch. >> > >> > As discussed, I am now storing the ref, DR and baseref, DR pairs along with >> unconditional read/write information in hash tables while iterating over DR >> during its initialization. >> > Then during checking for possible traps for if-converting, just check if the >> memory reference for a gimple statement is read/written unconditionally by >> querying the hash table instead of quadratic walk. >> > >> > Boot strapped and regression tested on x86_64. >> >> @@ -592,137 +598,153 @@ struct ifc_dr { >> >> /* -1 when not initialized, 0 when false, 1 when true. */ >> int rw_unconditionally; >> + >> + tree ored_result; >> + >> >> excess vertical space at the end. A better name would be simply "predicate". >> >> + if (!exsist1) >> + { >> + if (is_true_predicate (ca)) >> + { >> + DR_RW_UNCONDITIONALLY (a) = 1; >> + } >> >> - while (TREE_CODE (ref_base_a) == COMPONENT_REF >> - || TREE_CODE (ref_base_a) == IMAGPART_EXPR >> - || TREE_CODE (ref_base_a) == REALPART_EXPR) >> - ref_base_a = TREE_OPERAND (ref_base_a, 0); >> + IFC_DR (a)->ored_result = ca; >> + *master_dr = a; >> + } >> + else >> + { >> + IFC_DR (*master_dr)->ored_result >> + = fold_or_predicates >> + (EXPR_LOCATION (IFC_DR (*master_dr)->ored_result), >> + ca, IFC_DR (*master_dr)->ored_result); >> >> - while (TREE_CODE (ref_base_b) == COMPONENT_REF >> - || TREE_CODE (ref_base_b) == IMAGPART_EXPR >> - || TREE_CODE (ref_base_b) == REALPART_EXPR) >> - ref_base_b = TREE_OPERAND (ref_base_b, 0); >> + if (is_true_predicate (ca) >> + || is_true_predicate (IFC_DR (*master_dr)->ored_result)) >> + { >> + DR_RW_UNCONDITIONALLY (*master_dr) = 1; >> + } >> + } >> >> please common the predicate handling from both cases, that is, do >> >> if (is_true_predicate (IFC_DR (*master_dr)->ored_result)) >> DR_RW_... >> >> after the if. Note no {}s around single stmts. >> >> Likewise for the base_master_dr code. >> >> + if (!found) >> + { >> + DR_WRITTEN_AT_LEAST_ONCE (a) =0; >> + DR_RW_UNCONDITIONALLY (a) = 0; >> + return false; >> >> no need to update the flags on non-"master" DRs. >> >> Please remove the 'found' variable and simply return 'true' >> whenever found. >> >> static bool >> ifcvt_memrefs_wont_trap (gimple *stmt, vec refs) { >> - return write_memrefs_written_at_least_once (stmt, refs) >> - && memrefs_read_or_written_unconditionally (stmt, refs); >> + return memrefs_read_or_written_unconditionally (stmt, refs); >> >> please simply inline memrefs_read_or_written_unconditionally here. >> >> + if (ref_DR_map) >> + delete ref_DR_map; >> + ref_DR_map = NULL; >> + >> + if (baseref_DR_map) >> + delete baseref_DR_map; >> + baseref_DR_map = NULL; >> >> at this point the pointers will never be NULL. >> >> Ok with those changes. > > The attached patch addresses all the review comments and is rebased to current trunk. > GCC regression test and bootstrap passed. > > Wanted to check with you once before committing it to trunk. > Ok for trunk? > > gcc/ChangeLog > > 2015-11-15 Venkataramanan > * tree-if-conv.c (ref_DR_map): Define. > (baseref_DR_map): Like wise > (struct ifc_dr): Add new tree predicate field. > (hash_memrefs_baserefs_and_store_DRs_read_written_info): New function. > (memrefs_read_or_written_unconditionally): Use hash maps to query > unconditional read/written information. > (write_memrefs_written_at_least_once): Remove. > (ifcvt_memrefs_wont_trap): Remove call to > write_memrefs_written_at_least_once. > (if_convertible_loop_p_1): Initialize hash maps and predicates > before hashing data references. > * tree-data-ref.h (decl_binds_to_current_def_p): Declare . It's already declared in varasm.h which you need to include. You are correct in that we don't handle multiple data references in a single stmt in ifcvt_memrefs_wont_trap but that's a situation that cannot not happen. So it would be nice to make this clearer by changing the function to not loop over all DRs but instead just do data_reference_p a = drs[gimple_uid (stmt) - 1]; gcc_assert (DR_STMT (a) == stmt); instead of the for() loop. Ok with that change. Thanks, Richard. > > gcc/testsuite/ChangeLog > 2015-11-15 Venkataramanan > * gcc.dg/tree-ssa/ifc-8.c: Add new. > > > Regards, > Venkat. > >> >> Note the tree-hash-traits.h part is already committed. >> >> >> > gcc/ChangeLog >> > 2015-11-07 Venkataramanan >> > >> > *tree-hash-traits.h (struct tree_operand_hash). Use compare_type >> and value_type. >> > (equal_keys): Rename to equal and use compare_type and >> value_type. >> > * tree-if-conv.c (ref_DR_map): Define. >> > (baseref_DR_map): Like wise >> > (struct ifc_dr): New tree predicate field. >> > (hash_memrefs_baserefs_and_store_DRs_read_written_info): New >> function. >> > (memrefs_read_or_written_unconditionally): Use hashmap to query >> unconditional read/written information. >> > (write_memrefs_written_at_least_once) : Remove. >> > (ifcvt_memrefs_wont_trap): Remove call to >> write_memrefs_written_at_least_once. >> > (if_convertible_loop_p_1): Initialize/delete hash maps an initialize >> predicates >> > before the call to >> hash_memrefs_baserefs_and_store_DRs_read_written_info. >> >> Watch changelog formatting. >> >> Thanks, >> Richard. >> >> > gcc/testsuite/ChangeLog >> > 2015-11-07 Venkataramanan >> > *gcc.dg/tree-ssa/ifc-8.c: Add new. >> > >> > Ok for trunk? >> > >> > Regards, >> > Venkat. >> >> >> >> >> > + } >> >> >> > + } >> >> >> > + } >> >> >> > + return found; >> >> >> > +} >> >> >> > + >> >> >> > /* Return true when the memory references of STMT won't trap in >> the >> >> >> > if-converted code. There are two things that we have to check for: >> >> >> > >> >> >> > @@ -748,8 +776,20 @@ write_memrefs_written_at_least_once >> >> (gimple >> >> >> > *stmt, static bool ifcvt_memrefs_wont_trap (gimple *stmt, >> >> >> > vec refs) { >> >> >> > - return write_memrefs_written_at_least_once (stmt, refs) >> >> >> > - && memrefs_read_or_written_unconditionally (stmt, refs); >> >> >> > + bool memrefs_written_once, >> >> memrefs_read_written_unconditionally; >> >> >> > + bool memrefs_safe_to_access; >> >> >> > + >> >> >> > + memrefs_written_once >> >> >> > + = write_memrefs_written_at_least_once (stmt, >> >> >> > + refs); >> >> >> > + >> >> >> > + memrefs_read_written_unconditionally >> >> >> > + = memrefs_read_or_written_unconditionally (stmt, >> >> >> > + refs); >> >> >> > + >> >> >> > + memrefs_safe_to_access >> >> >> > + = write_memrefs_safe_to_access_unconditionally >> >> >> > + (stmt, refs); >> >> >> > + >> >> >> > + return ((memrefs_written_once || memrefs_safe_to_access) >> >> >> > + && memrefs_read_written_unconditionally); >> >> >> > } >> >> >> > >> >> >> > /* Wrapper around gimple_could_trap_p refined for the needs of >> >> >> > the >> >> >> > >> >> >> > >> >> >> > do I need this function write_memrefs_written_at_least_once >> >> anymore? >> >> >> > Please suggest if there a better way to do this. >> >> >> > >> >> >> > Bootstrapped and regression tested on x86_64. >> >> >> > I am not adding change log and comments now, as I wanted to >> >> >> > check >> >> >> approach first. >> >> >> > >> >> >> > Regards, >> >> >> > Venkat. >> >> >> > >> >> >> > >> >