| Message ID | CY1PR1201MB109888741196B084079723C78F1A0@CY1PR1201MB1098.namprd12.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 20, 2015 at 12:02:07PM +0000, Kumar, Venkataramanan wrote: > Also. > (1) I guard these checks for -ftree-loop-if-convert-stores and -fno-common. > Sometimes vectorization flags also triggers if conversion. > (2) Also hashing base DRs for writes only. Let me comment just on formatting, will leave actual review to Richard. > > gcc/ChangeLog > 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> Surname missing. > > PR tree-optimization/67326 > * tree-if-conv.c (offset_DR_map): Define. Extraneous space. > (struct ifc_dr): Add new tree base_predicate field. All ChangeLog lines should be indented by a single tab. > (hash_memrefs_baserefs_and_store_DRs_read_written_info): Hash offsets, DR pairs Too long line. > and hash base ref, DR pairs for write type DRs. Extraneous spaces. > (ifcvt_memrefs_wont_trap): Guard checks with -ftree-loop-if-convert-stores flag. Too long line and extraneous spaces. > Check for similar DR that are accessed unconditionally. > (if_convertible_loop_p_1): Initialize and delete offset hash maps Extraneous space, missing full stop at the end. > > gcc/testsuite/ChangeLog > 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> > * gcc.dg/tree-ssa/ifc-pr67326.c: Add new. Extraneous space. > + if (DR_IS_WRITE (a)) > { > - IFC_DR (a)->predicate = ca; > - *base_master_dr = a; > + base_master_dr = &baseref_DR_map->get_or_insert (base_ref,&exist2); Missing space before &exist2); > + offset_master_dr = &offset_DR_map->get_or_insert (offset,&exist3); > + if (!exist3) > + *offset_master_dr = a; > + > + if (DR_RW_UNCONDITIONALLY (*offset_master_dr) != 1) > + DR_RW_UNCONDITIONALLY (*offset_master_dr) > + = DR_RW_UNCONDITIONALLY (*master_dr); Wrong indentation, the = should be below the first underscore in DR_RW_UNCONDITIONALLY. > - if (DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) == 1) > + if ((base_master_dr > + && DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) == 1)) Extraneous () pair. > + else if (DR_OFFSET (a)) > + { > + offset_dr = offset_DR_map->get (DR_OFFSET (a)); > + if ((DR_RW_UNCONDITIONALLY (*offset_dr) == 1) Likewise. Jakub
On Fri, Nov 20, 2015 at 1:02 PM, Kumar, Venkataramanan <Venkataramanan.Kumar@amd.com> wrote: > Hi Richard, > > As per Jakub suggestion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67326, the below patch fixes the regression in tree if conversion. > Basically allowing if conversion to happen for a candidate DR, if we find similar DR with same dimensions and that DR will not trap. > > To find similar DRs using hash table to hashing the offset and DR pairs. > Also reusing read/written information that was stored for reference tree. > > Also. > (1) I guard these checks for -ftree-loop-if-convert-stores and -fno-common. > Sometimes vectorization flags also triggers if conversion. > (2) Also hashing base DRs for writes only. > > gcc/ChangeLog > 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> > > PR tree-optimization/67326 > * tree-if-conv.c (offset_DR_map): Define. > (struct ifc_dr): Add new tree base_predicate field. > (hash_memrefs_baserefs_and_store_DRs_read_written_info): Hash offsets, DR pairs > and hash base ref, DR pairs for write type DRs. > (ifcvt_memrefs_wont_trap): Guard checks with -ftree-loop-if-convert-stores flag. > Check for similar DR that are accessed unconditionally. > (if_convertible_loop_p_1): Initialize and delete offset hash maps > > gcc/testsuite/ChangeLog > 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> > * gcc.dg/tree-ssa/ifc-pr67326.c: Add new. > > Regstrapped on x86_64, Ok for trunk? + if (offset) + { + offset_master_dr = &offset_DR_map->get_or_insert (offset,&exist3); + if (!exist3) + *offset_master_dr = a; + + if (DR_RW_UNCONDITIONALLY (*offset_master_dr) != 1) + DR_RW_UNCONDITIONALLY (*offset_master_dr) + = DR_RW_UNCONDITIONALLY (*master_dr); this is fishy - as far as I can see offset_master globs all _candidates_ and + else if (DR_OFFSET (a)) + { + offset_dr = offset_DR_map->get (DR_OFFSET (a)); + if ((DR_RW_UNCONDITIONALLY (*offset_dr) == 1) + && DR_NUM_DIMENSIONS (a) == DR_NUM_DIMENSIONS (*offset_dr)) + { + tree base_tree = get_base_address (DR_REF (a)); + if (DECL_P (base_tree) + && flag_tree_loop_if_convert_stores + && decl_binds_to_current_def_p (base_tree) + && !TREE_READONLY (base_tree)) + return true; + } + } where with this that actually checks something (DR_NUM_DIMENSIONS is not something you can use to identify two arrays with the same domain) will then consider DR_DW_UNCONDITIONALLY ORed from all _candidates_ but not only from those which really have the same domain. You need to do the domain check as part of the hash-map hashing/comparing. Note that there is no bounds info in the data ref info so you need to a) consider DR_OFFSET + DR_INIT b) verify the access size is the same (TYPE_SIZE_UNIT (TREE_TYPE (dr->ref))) c) verify the base objects are of the same size - note this is somewhat difficult as the base object for DR_OFFSET/INIT is starting at DR_BASE_ADDRESS so maybe restrict this to ADDR_EXPR <decl> DR_BASE_ADDRESS cases where you can look at DECL_SIZE (decl) of both candidates You can also try using indices (DR_BASE_OBJECT plus DR_ACCESS_FNS when DR_UNCONSTRAINED_BASE is false). If the size of DR_BASE_OBJECT matches and all access functions are equal it should be a compatible enough case as well. I'd say you should split out the base_predicate introduction into a separate patch (this change looks ok). Richard. > Regards, > Venkat. >
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr67326.c b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr67326.c new file mode 100644 index 0000000..5ca11cd --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr67326.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-Ofast -fdump-tree-ifcvt-details -fno-common -ftree-loop-if-convert-stores" } */ + +float v0[1024]; +float v1[1024]; +float v2[1024]; +float v3[1024]; + +void condAssign1 () { + for (int i=0; i<1024; ++i) + v0[i] = (v2[i]>v1[i]) ? v2[i]*v3[i] : v0[i]; +} + +void condAssign2 () { + for (int i=0; i<1024; ++i) + v0[i] = (v2[i]>v1[i]) ? v2[i]*v1[i] : v0[i]; +} + +/* { dg-final { scan-tree-dump-times "Applying if-conversion" 2 "ifcvt" } } */ diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c index 01065cb..1b4d220 100644 --- a/gcc/tree-if-conv.c +++ b/gcc/tree-if-conv.c @@ -124,6 +124,9 @@ static hash_map<tree_operand_hash, data_reference_p> *ref_DR_map; /* Hash table to store base reference, DR pairs. */ static hash_map<tree_operand_hash, data_reference_p> *baseref_DR_map; +/* Hash table to store DR offset, DR pairs. */ +static hash_map<tree_operand_hash, data_reference_p> *offset_DR_map; + /* Structure used to predicate basic blocks. This is attached to the ->aux field of the BBs in the loop to be if-converted. */ struct bb_predicate { @@ -589,6 +592,8 @@ struct ifc_dr { int rw_unconditionally; tree predicate; + + tree base_predicate; }; #define IFC_DR(DR) ((struct ifc_dr *) (DR)->aux) @@ -609,11 +614,12 @@ static void hash_memrefs_baserefs_and_store_DRs_read_written_info (data_reference_p a) { - data_reference_p *master_dr, *base_master_dr; + data_reference_p *master_dr, *base_master_dr, *offset_master_dr; tree ref = DR_REF (a); tree base_ref = DR_BASE_OBJECT (a); + tree offset = DR_OFFSET (a); tree ca = bb_predicate (gimple_bb (DR_STMT (a))); - bool exist1, exist2; + bool exist1, exist2, exist3; while (TREE_CODE (ref) == COMPONENT_REF || TREE_CODE (ref) == IMAGPART_EXPR @@ -636,22 +642,34 @@ hash_memrefs_baserefs_and_store_DRs_read_written_info (data_reference_p a) if (is_true_predicate (IFC_DR (*master_dr)->predicate)) DR_RW_UNCONDITIONALLY (*master_dr) = 1; - base_master_dr = &baseref_DR_map->get_or_insert (base_ref,&exist2); - - if (!exist2) + if (DR_IS_WRITE (a)) { - IFC_DR (a)->predicate = ca; - *base_master_dr = a; + base_master_dr = &baseref_DR_map->get_or_insert (base_ref,&exist2); + if (!exist2) + { + IFC_DR (a)->base_predicate = ca; + *base_master_dr = a; + } + else + IFC_DR (*base_master_dr)->base_predicate + = fold_or_predicates + (EXPR_LOCATION (IFC_DR (*base_master_dr)->base_predicate), + ca, IFC_DR (*base_master_dr)->base_predicate); + + if (is_true_predicate (IFC_DR (*base_master_dr)->base_predicate)) + DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) = 1; } - else - IFC_DR (*base_master_dr)->predicate - = fold_or_predicates - (EXPR_LOCATION (IFC_DR (*base_master_dr)->predicate), - ca, IFC_DR (*base_master_dr)->predicate); - if (DR_IS_WRITE (a) - && (is_true_predicate (IFC_DR (*base_master_dr)->predicate))) - DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) = 1; + if (offset) + { + offset_master_dr = &offset_DR_map->get_or_insert (offset,&exist3); + if (!exist3) + *offset_master_dr = a; + + if (DR_RW_UNCONDITIONALLY (*offset_master_dr) != 1) + DR_RW_UNCONDITIONALLY (*offset_master_dr) + = DR_RW_UNCONDITIONALLY (*master_dr); + } } /* Return true when the memory references of STMT won't trap in the @@ -682,7 +700,7 @@ hash_memrefs_baserefs_and_store_DRs_read_written_info (data_reference_p a) static bool ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> drs) { - data_reference_p *master_dr, *base_master_dr; + data_reference_p *master_dr, *base_master_dr, *offset_dr; data_reference_p a = drs[gimple_uid (stmt) - 1]; tree ref_base_a = DR_REF (a); @@ -698,21 +716,38 @@ ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> drs) master_dr = ref_DR_map->get (ref_base_a); base_master_dr = baseref_DR_map->get (base); - gcc_assert (master_dr != NULL && base_master_dr != NULL); + gcc_assert (master_dr != NULL); if (DR_RW_UNCONDITIONALLY (*master_dr) == 1) { - if (DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) == 1) + if ((base_master_dr + && DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) == 1)) return true; else { tree base_tree = get_base_address (DR_REF (a)); if (DECL_P (base_tree) && decl_binds_to_current_def_p (base_tree) + && flag_tree_loop_if_convert_stores && !TREE_READONLY (base_tree)) return true; } } + else if (DR_OFFSET (a)) + { + offset_dr = offset_DR_map->get (DR_OFFSET (a)); + if ((DR_RW_UNCONDITIONALLY (*offset_dr) == 1) + && DR_NUM_DIMENSIONS (a) == DR_NUM_DIMENSIONS (*offset_dr)) + { + tree base_tree = get_base_address (DR_REF (a)); + if (DECL_P (base_tree) + && flag_tree_loop_if_convert_stores + && decl_binds_to_current_def_p (base_tree) + && !TREE_READONLY (base_tree)) + return true; + } + } + return false; } @@ -1267,6 +1302,7 @@ if_convertible_loop_p_1 (struct loop *loop, ref_DR_map = new hash_map<tree_operand_hash, data_reference_p>; baseref_DR_map = new hash_map<tree_operand_hash, data_reference_p>; + offset_DR_map = new hash_map<tree_operand_hash, data_reference_p>; predicate_bbs (loop); @@ -1382,6 +1418,9 @@ if_convertible_loop_p (struct loop *loop, bool *any_mask_load_store) delete baseref_DR_map; baseref_DR_map = NULL; + delete offset_DR_map; + offset_DR_map = NULL; + return res; }
Hi Richard, As per Jakub suggestion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67326, the below patch fixes the regression in tree if conversion. Basically allowing if conversion to happen for a candidate DR, if we find similar DR with same dimensions and that DR will not trap. To find similar DRs using hash table to hashing the offset and DR pairs. Also reusing read/written information that was stored for reference tree. Also. (1) I guard these checks for -ftree-loop-if-convert-stores and -fno-common. Sometimes vectorization flags also triggers if conversion. (2) Also hashing base DRs for writes only. gcc/ChangeLog 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> PR tree-optimization/67326 * tree-if-conv.c (offset_DR_map): Define. (struct ifc_dr): Add new tree base_predicate field. (hash_memrefs_baserefs_and_store_DRs_read_written_info): Hash offsets, DR pairs and hash base ref, DR pairs for write type DRs. (ifcvt_memrefs_wont_trap): Guard checks with -ftree-loop-if-convert-stores flag. Check for similar DR that are accessed unconditionally. (if_convertible_loop_p_1): Initialize and delete offset hash maps gcc/testsuite/ChangeLog 2015-11-19 Venkataramanan <Venkataramanan.Kumar@amd.com> * gcc.dg/tree-ssa/ifc-pr67326.c: Add new. Regstrapped on x86_64, Ok for trunk? Regards, Venkat.