This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


So I have modified the patch to use hash_map instead of std::map. The patch is attached.

However, I got one regression after that.

# Comparing directories
## Dir1=../build-pristine/: 11 sum files
## Dir2=../build-test/: 11 sum files

# Comparing 11 common sum files
## /bin/sh ../contrib/compare_tests  /tmp/gxx-sum1.29214 /tmp/gxx-sum2.29214
Tests that now fail, but worked before:

c-c++-common/goacc/loop-private-1.c  -std=c++98  scan-tree-dump-times gimple "#pragma acc loop collapse\\(2\\) private\\(j\\) private\\(i\\)" 1
c-c++-common/goacc/loop-private-1.c scan-tree-dump-times gimple "#pragma acc loop collapse\\(2\\) private\\(j\\) private\\(i\\)" 1

## Differences found: 
# 1 differences in 11 common sum files found


The error is due to mis-comparison because of reordering of private(i) private(j).

I wanted to know if the order of the private flags matter.


Thanks,
-Aditya


----------------------------------------
> From: hiraditya@msn.com
> To: prathamesh.kulkarni@linaro.org
> CC: richard.guenther@gmail.com; tbsaunde@tbsaunde.org; gcc@gcc.gnu.org
> Subject: RE: Proposal for adding splay_tree_find (to find elements without updating the nodes).
> Date: Wed, 18 Mar 2015 18:14:49 +0000
>
>
>
> ----------------------------------------
>> Date: Wed, 18 Mar 2015 22:01:18 +0530
>> Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
>> From: prathamesh.kulkarni@linaro.org
>> To: hiraditya@msn.com
>> CC: richard.guenther@gmail.com; tbsaunde@tbsaunde.org; gcc@gcc.gnu.org
>>
>> 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
>>
>
> Thanks for letting me know.
> -Aditya
>
>> 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!)
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>
 		 	   		  

Attachment: splay_tree.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]