RE: [RFC] [Patch] PR67326 - relax trap assumption by looking at similar DRS

["Kumar, Venkataramanan" <Venkataramanan dot Kumar at amd dot com>]

Hi Richard,

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, November 24, 2015 9:07 PM
> To: Kumar, Venkataramanan
> Cc: Jakub Jelinek (jakub@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.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.

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.Kumar@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

Attachment: add.base.predicate.patch.txt
Description: add.base.predicate.patch.txt