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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Nov 27, 2015 at 12:24 AM, Kumar, Venkataramanan
<Venkataramanan.Kumar@amd.com> wrote:
> 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

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68621

-- 
H.J.