This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
- From: Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>
- To: Aditya K <hiraditya at msn dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, Trevor Saunders <tbsaunde at tbsaunde dot org>, "gcc at gcc dot gnu dot org" <gcc at gcc dot gnu dot org>
- Date: Wed, 18 Mar 2015 22:01:18 +0530
- Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
- Authentication-results: sourceware.org; auth=none
- References: <BLU179-W40C136025D7BD569ABEE24B61B0 at phx dot gbl> <CABu31nMN2KajAC4LO=RBrmxG9gHapMJmWbBC=8i_i9RyieZ5pA at mail dot gmail dot com> <CAFiYyc3uGtRxXN_bdTSHNeBTCX41JnNT3SYs2CAyYXs0_2sR5g at mail dot gmail dot com> <BLU179-W86E1C1E984B9AE31FCC5DBB6070 at phx dot gbl> <CAH6eHdRWUfuVEac-4aZTayBCr5BmbzELNPEz62-FNpr44ihksw at mail dot gmail dot com> <BLU179-W229A566D6DBF2668CBB6ACB6070 at phx dot gbl> <20150315060335 dot GA17414 at tsaunders-iceball dot corp dot tor1 dot mozilla dot com> <BLU179-W61338647FCB5BE476609FB6020 at phx dot gbl> <CAFiYyc3tbBRUT2Jof5qDi=aeL7Zy4hcb5DQoHa6CwJ9_FAS0PA at mail dot gmail dot com> <BLU179-W3054B2B4026817EFC4905FB6000 at phx dot gbl>
On 18 March 2015 at 21:20, Aditya K <hiraditya@msn.com> wrote:
>
>
> ----------------------------------------
>> Date: Wed, 18 Mar 2015 11:50:16 +0100
>> Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
>> From: richard.guenther@gmail.com
>> To: hiraditya@msn.com
>> CC: tbsaunde@tbsaunde.org; gcc@gcc.gnu.org
>>
>> On Mon, Mar 16, 2015 at 4:33 AM, Aditya K <hiraditya@msn.com> 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;
>>>
>>>
>>>> -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.
>>
>> I'm not sure we want to use std::map. Can you use GCCs own hash_map
>> here?
>
> Ok, I'll try to use has_map. I was under the impression that we can use standard library features, that's why I used std::map.
>
Using std::map caused a bootstrap build problem on AIX.
see: https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02608.html
However I am not sure if that's true any more after the following fix
was commmited:
https://gcc.gnu.org/ml/libstdc++/2014-10/msg00195.html
Regards,
Prathamesh
> Thanks,
> -Aditya
>
>>
>> Richard.
>>
>>>
>>> -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!)
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>