Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Mon, Mar 16, 2015 at 03:33:18AM +0000, Aditya K wrote:
> 
> 
> ----------------------------------------
> > Date: Sun, 15 Mar 2015 02:32:23 -0400
> > From: tbsaunde@tbsaunde.org
> > To: gcc@gcc.gnu.org
> > Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
> >
> > hi,
> >
> > I'm only commenting on algorithmic stuff at this point, you should make
> > sure this doesn't regress anything in make check. This stuff only
> > effects code using omp stuff so compiling random c++ is unlikely to test
> > this code at all.
> >
> > Also please follow the style in
> > https://gcc.gnu.org/codingconventions.html
> > and usually try to make new code similar to what's around it.
> >
> > @@ -384,7 +386,7 @@ new_omp_context (enum omp_region_type region_type)
> >
> > c = XCNEW (struct gimplify_omp_ctx);
> > c->outer_context = gimplify_omp_ctxp;
> > - c->variables = splay_tree_new (splay_tree_compare_decl_uid, 0, 0);
> > + //c->variables = splay_tree_new (splay_tree_compare_decl_uid, 0, 0);
> >
> > I don't think this is what you want, xcnew is a calloc wrapper and
> > doesn't call the ctor for gimplify_omp_ctx. For now placement new is
> > probably the simplest way to get what you want.
> >
> Thanks for pointing this out. I'll do it the way c->privatized_types has been allocated.
> e.g., by making c->variables a pointer to std::map and   c->variables = new gimplify_tree_t;

that works too, though I'd be a more descriptive name than
gimplify_tree_t, maybe omp_variables_map?

> 
> 
> > -static void
> > -delete_omp_context (struct gimplify_omp_ctx *c)
> > -{
> > - splay_tree_delete (c->variables);
> > - delete c->privatized_types;
> > - XDELETE (c);
> > -}
> >
> > hm, why?
> >
> My bad, I'll restore this.
> 
> > -gimplify_adjust_omp_clauses_1 (splay_tree_node n, void *data)
> > +gimplify_adjust_omp_clauses_1 (std::pair<tree, unsigned> n, void *data)
> >
> > You can now change the type of data from void * to const
> > gimplify_adjust_omp_clauses_data *
> 
> Done!
> 
> 
> Thanks for the feedback, they were really helpful. I have updated the patch. Please review this.
> Also, although I run `make check` while compiling gcc (with bootstrap enabled), I'm not sure if 'omp' related tests were exercised.
> I'm still unfamiliar with several components of gcc. Any pointers on how to ensure all tests were run, would be useful.


https://gcc.gnu.org/install/test.html should help, though unfortunately
you'll probably find the easiest way to check for regressions is to do
one run of straight trunk, then another with your patch.  Saddly a bunch
of people have own scripts to deal with administrivia, but there isn't a
standardized way that's simple.

Trev

> 
> 
> -Aditya
> 
> 
> 
> 
> >
> > thanks!
> >
> > Trev
> >
> > On Fri, Mar 13, 2015 at 07:32:03PM +0000, Aditya K wrote:
> >> You're right. I'll change this to:
> >>
> >> /* A stable comparison functor to sort trees.  */
> >> struct tree_compare_decl_uid {
> >>   bool  operator ()(const tree &xa, const tree &xb) const
> >>   {
> >>     return DECL_UID (xa) < DECL_UID (xb);
> >>   }
> >> };
> >>
> >> New patch attached.
> >>
> >>
> >> Thanks,
> >> -Aditya
> >>
> >>
> >> ----------------------------------------
> >>> Date: Fri, 13 Mar 2015 19:02:11 +0000
> >>> Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
> >>> From: jwakely.gcc@gmail.com
> >>> To: hiraditya@msn.com
> >>> CC: richard.guenther@gmail.com; stevenb.gcc@gmail.com; gcc@gcc.gnu.org
> >>>
> >>> Are you sure your compare_variables functor is correct?
> >>>
> >>> Subtracting the two values seems very strange for a strict weak ordering.
> >>>
> >>> (Also "compare_variables" is a pretty poor name!)
> >>
> >
> >
> 
> 
>