On Sun, Nov 15, 2015 at 12:14 PM, Kumar, Venkataramanan
<venkataramanan.ku...@amd.com> wrote:
> Hi Richard and Bernhard.
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guent...@gmail.com]
>> Sent: Tuesday, November 10, 2015 5:33 PM
>> To: Kumar, Venkataramanan
>> Cc: Andrew Pinski; gcc-patches@gcc.gnu.org
>> Subject: Re: [RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap
>> assumptions.
>>
>> On Sat, Nov 7, 2015 at 12:41 PM, Kumar, Venkataramanan
>> <venkataramanan.ku...@amd.com> wrote:
>> > Hi Richard,
>> >
>> > I have now implemented storing of DR and references using hash maps.
>> > Please find attached patch.
>> >
>> > As discussed, I am now storing the ref, DR and baseref, DR pairs along
>> > with
>> unconditional read/write information in hash tables while iterating over DR
>> during its initialization.
>> > Then during checking for possible traps for if-converting, just check if
>> > the
>> memory reference for a gimple statement is read/written unconditionally by
>> querying the hash table instead of quadratic walk.
>> >
>> > Boot strapped and regression tested on x86_64.
>>
>> @@ -592,137 +598,153 @@ struct ifc_dr {
>>
>> /* -1 when not initialized, 0 when false, 1 when true. */
>> int rw_unconditionally;
>> +
>> + tree ored_result;
>> +
>>
>> excess vertical space at the end. A better name would be simply "predicate".
>>
>> + if (!exsist1)
>> + {
>> + if (is_true_predicate (ca))
>> + {
>> + DR_RW_UNCONDITIONALLY (a) = 1;
>> + }
>>
>> - while (TREE_CODE (ref_base_a) == COMPONENT_REF
>> - || TREE_CODE (ref_base_a) == IMAGPART_EXPR
>> - || TREE_CODE (ref_base_a) == REALPART_EXPR)
>> - ref_base_a = TREE_OPERAND (ref_base_a, 0);
>> + IFC_DR (a)->ored_result = ca;
>> + *master_dr = a;
>> + }
>> + else
>> + {
>> + IFC_DR (*master_dr)->ored_result
>> + = fold_or_predicates
>> + (EXPR_LOCATION (IFC_DR (*master_dr)->ored_result),
>> + ca, IFC_DR (*master_dr)->ored_result);
>>
>> - while (TREE_CODE (ref_base_b) == COMPONENT_REF
>> - || TREE_CODE (ref_base_b) == IMAGPART_EXPR
>> - || TREE_CODE (ref_base_b) == REALPART_EXPR)
>> - ref_base_b = TREE_OPERAND (ref_base_b, 0);
>> + if (is_true_predicate (ca)
>> + || is_true_predicate (IFC_DR (*master_dr)->ored_result))
>> + {
>> + DR_RW_UNCONDITIONALLY (*master_dr) = 1;
>> + }
>> + }
>>
>> please common the predicate handling from both cases, that is, do
>>
>> if (is_true_predicate (IFC_DR (*master_dr)->ored_result))
>> DR_RW_...
>>
>> after the if. Note no {}s around single stmts.
>>
>> Likewise for the base_master_dr code.
>>
>> + if (!found)
>> + {
>> + DR_WRITTEN_AT_LEAST_ONCE (a) =0;
>> + DR_RW_UNCONDITIONALLY (a) = 0;
>> + return false;
>>
>> no need to update the flags on non-"master" DRs.
>>
>> Please remove the 'found' variable and simply return 'true'
>> whenever found.
>>
>> static bool
>> ifcvt_memrefs_wont_trap (gimple *stmt, vec<data_reference_p> refs) {
>> - return write_memrefs_written_at_least_once (stmt, refs)
>> - && memrefs_read_or_written_unconditionally (stmt, refs);
>> + return memrefs_read_or_written_unconditionally (stmt, refs);
>>
>> please simply inline memrefs_read_or_written_unconditionally here.
>>
>> + if (ref_DR_map)
>> + delete ref_DR_map;
>> + ref_DR_map = NULL;
>> +
>> + if (baseref_DR_map)
>> + delete baseref_DR_map;
>> + baseref_DR_map = NULL;
>>
>> at this point the pointers will never be NULL.
>>
>> Ok with those changes.
>
> The attached patch addresses all the review comments and is rebased to
> current trunk.
> GCC regression test and bootstrap passed.
>
> Wanted to check with you once before committing it to trunk.
> Ok for trunk?
>
> gcc/ChangeLog
>
> 2015-11-15 Venkataramanan <venkataramanan.ku...@amd.com>
> * tree-if-conv.c (ref_DR_map): Define.
> (baseref_DR_map): Like wise
> (struct ifc_dr): Add new tree predicate field.
> (hash_memrefs_baserefs_and_store_DRs_read_written_info): New function.
> (memrefs_read_or_written_unconditionally): Use hash maps to query
> unconditional read/written information.
> (write_memrefs_written_at_least_once): Remove.
> (ifcvt_memrefs_wont_trap): Remove call to
> write_memrefs_written_at_least_once.
> (if_convertible_loop_p_1): Initialize hash maps and predicates
> before hashing data references.
> * tree-data-ref.h (decl_binds_to_current_def_p): Declare .
It's already declared in varasm.h which you need to include.
You are correct in that we don't handle multiple data references in a
single stmt
in ifcvt_memrefs_wont_trap but that's a situation that cannot not happen.
So it would be nice to make this clearer by changing the function to
not loop over
all DRs but instead just do
data_reference_p a = drs[gimple_uid (stmt) - 1];
gcc_assert (DR_STMT (a) == stmt);
instead of the for() loop.
Ok with that change.
Thanks,
Richard.
>
> gcc/testsuite/ChangeLog
> 2015-11-15 Venkataramanan <venkataramanan.ku...@amd.com>
> * gcc.dg/tree-ssa/ifc-8.c: Add new.
>
>
> Regards,
> Venkat.
>
>>
>> Note the tree-hash-traits.h part is already committed.
>>
>>
>> > gcc/ChangeLog
>> > 2015-11-07 Venkataramanan <venkataramanan.ku...@amd.com>
>> >
>> > *tree-hash-traits.h (struct tree_operand_hash). Use compare_type
>> and value_type.
>> > (equal_keys): Rename to equal and use compare_type and
>> value_type.
>> > * tree-if-conv.c (ref_DR_map): Define.
>> > (baseref_DR_map): Like wise
>> > (struct ifc_dr): New tree predicate field.
>> > (hash_memrefs_baserefs_and_store_DRs_read_written_info): New
>> function.
>> > (memrefs_read_or_written_unconditionally): Use hashmap to query
>> unconditional read/written information.
>> > (write_memrefs_written_at_least_once) : Remove.
>> > (ifcvt_memrefs_wont_trap): Remove call to
>> write_memrefs_written_at_least_once.
>> > (if_convertible_loop_p_1): Initialize/delete hash maps an
>> > initialize
>> predicates
>> > before the call to
>> hash_memrefs_baserefs_and_store_DRs_read_written_info.
>>
>> Watch changelog formatting.
>>
>> Thanks,
>> Richard.
>>
>> > gcc/testsuite/ChangeLog
>> > 2015-11-07 Venkataramanan <venkataramanan.ku...@amd.com>
>> > *gcc.dg/tree-ssa/ifc-8.c: Add new.
>> >
>> > Ok for trunk?
>> >
>> > Regards,
>> > Venkat.
>> >>
>> >> >> > + }
>> >> >> > + }
>> >> >> > + }
>> >> >> > + return found;
>> >> >> > +}
>> >> >> > +
>> >> >> > /* Return true when the memory references of STMT won't trap in
>> the
>> >> >> > if-converted code. There are two things that we have to check
>> >> >> > for:
>> >> >> >
>> >> >> > @@ -748,8 +776,20 @@ write_memrefs_written_at_least_once
>> >> (gimple
>> >> >> > *stmt, static bool ifcvt_memrefs_wont_trap (gimple *stmt,
>> >> >> > vec<data_reference_p> refs) {
>> >> >> > - return write_memrefs_written_at_least_once (stmt, refs)
>> >> >> > - && memrefs_read_or_written_unconditionally (stmt, refs);
>> >> >> > + bool memrefs_written_once,
>> >> memrefs_read_written_unconditionally;
>> >> >> > + bool memrefs_safe_to_access;
>> >> >> > +
>> >> >> > + memrefs_written_once
>> >> >> > + = write_memrefs_written_at_least_once (stmt,
>> >> >> > + refs);
>> >> >> > +
>> >> >> > + memrefs_read_written_unconditionally
>> >> >> > + = memrefs_read_or_written_unconditionally (stmt,
>> >> >> > + refs);
>> >> >> > +
>> >> >> > + memrefs_safe_to_access
>> >> >> > + = write_memrefs_safe_to_access_unconditionally
>> >> >> > + (stmt, refs);
>> >> >> > +
>> >> >> > + return ((memrefs_written_once || memrefs_safe_to_access)
>> >> >> > + && memrefs_read_written_unconditionally);
>> >> >> > }
>> >> >> >
>> >> >> > /* Wrapper around gimple_could_trap_p refined for the needs of
>> >> >> > the
>> >> >> >
>> >> >> >
>> >> >> > do I need this function write_memrefs_written_at_least_once
>> >> anymore?
>> >> >> > Please suggest if there a better way to do this.
>> >> >> >
>> >> >> > Bootstrapped and regression tested on x86_64.
>> >> >> > I am not adding change log and comments now, as I wanted to
>> >> >> > check
>> >> >> approach first.
>> >> >> >
>> >> >> > Regards,
>> >> >> > Venkat.
>> >> >> >
>> >> >> >
>> >
