Hi Richard,
> -----Original Message-----
> From: Richard Biener [mailto:richard.guent...@gmail.com]
> Sent: Tuesday, November 24, 2015 9:07 PM
> To: Kumar, Venkataramanan
> Cc: Jakub Jelinek (ja...@redhat.com); gcc-patches@gcc.gnu.org
> Subject: Re: [RFC] [Patch] PR67326 - relax trap assumption by looking at
> similar DRS
>
> On Fri, Nov 20, 2015 at 1:02 PM, Kumar, Venkataramanan
> <venkataramanan.ku...@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.ku...@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.ku...@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.
Ok, I will take some time to figure out on domain analysis part.
>
> I'd say you should split out the base_predicate introduction into a separate
> patch (this change looks ok).
>
Attached patch has the "base_predicate" introduction part alone.
It does the predicate folding and hashes base references for only write type
DRs while hashing.
I have not added any new test case since we already have ifc-8.c
Also fixed formatting issues Jakub pointed out for this patch.
Boot strapped on X86_64.
Ok to upstream if it passes regression tests?
gcc/ChangeLog
2015-11-27 Venkataramanan Kumar <venkataramanan.ku...@amd.com>
* tree-if-conv.c (struct ifc_dr): Add new tree
base_predicate field.
(hash_memrefs_baserefs_and_store_DRs_read_written_info): Hash
base ref, DR pairs and store base_predicate for write type DRs.
(ifcvt_memrefs_wont_trap): Guard checks with
-ftree-loop-if-convert-stores flag
Regards,
Venkat
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 01065cb..f43942d 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -589,6 +589,8 @@ struct ifc_dr {
int rw_unconditionally;
tree predicate;
+
+ tree base_predicate;
};
#define IFC_DR(DR) ((struct ifc_dr *) (DR)->aux)
@@ -636,22 +638,24 @@ 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;
- }
- 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);
+ base_master_dr = &baseref_DR_map->get_or_insert (base_ref, &exist2);
- if (DR_IS_WRITE (a)
- && (is_true_predicate (IFC_DR (*base_master_dr)->predicate)))
- DR_WRITTEN_AT_LEAST_ONCE (*base_master_dr) = 1;
+ 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;
+ }
}
/* Return true when the memory references of STMT won't trap in the
@@ -698,17 +702,19 @@ 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;
}
