Hi Richard,
> -----Original Message----- > From: Richard Biener [mailto:richard.guent...@gmail.com] > Sent: Monday, November 16, 2015 3:28 PM > To: Kumar, Venkataramanan > Cc: Bernhard Reutner-Fischer; Andrew Pinski; gcc-patches@gcc.gnu.org > Subject: Re: [RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap > assumptions. > > On Sun, Nov 15, 2015 at 12:14 PM, Kumar, Venkataramanan > <venkataramanan.ku...@amd.com> wrote: > > Hi Richard and Bernhard. > > > >> -----Original Message----- > >> From: Richard Biener [mailto:richard.guent...@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 > >> <venkataramanan.ku...@amd.com> 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<data_reference_p> 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 <venkataramanan.ku...@amd.com> > > * 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. Up streamed the changes after Boot strap and regression testing on X86_64 target. Ref: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=230454 Regards, Venkat. > > Thanks, > Richard. > > > > > gcc/testsuite/ChangeLog > > 2015-11-15 Venkataramanan <venkataramanan.ku...@amd.com> > > * 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 <venkataramanan.ku...@amd.com> > >> > > >> > *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 <venkataramanan.ku...@amd.com> > >> > *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<data_reference_p> 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. > >> >> >> > > >> >> >> > > >> >
