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

[Richard Biener <richard dot guenther at gmail dot com>]

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