[RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap assumptions.

Kumar, Venkataramanan Venkataramanan.Kumar@amd.com
Tue Nov 17 07:48:00 GMT 2015


Hi Richard,


> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Monday, November 16, 2015 3:28 PM
> To: Kumar, Venkataramanan
> Cc: Bernhard Reutner-Fischer; Andrew Pinski; gcc-patches@gcc.gnu.org
> Subject: Re: [RFC] [PATCH V2]: RE: [RFC] [Patch] Relax tree-if-conv.c trap
> assumptions.
> 
> On Sun, Nov 15, 2015 at 12:14 PM, Kumar, Venkataramanan
> <Venkataramanan.Kumar@amd.com> wrote:
> > Hi Richard  and Bernhard.
> >
> >> -----Original Message-----
> >> From: Richard Biener [mailto:richard.guenther@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.Kumar@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.Kumar@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.

Up streamed  the changes after Boot strap and regression testing on X86_64 target. 

Ref: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=230454

Regards,
Venkat.

> 
> Thanks,
> Richard.
> 
> >
> > gcc/testsuite/ChangeLog
> > 2015-11-15  Venkataramanan  <Venkataramanan.Kumar@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.Kumar@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.Kumar@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.
> >> >> >> >
> >> >> >> >
> >> >


More information about the Gcc-patches mailing list