[patch] Privatize gimplify_ctx structure.

[Andrew MacLeod]

On 11/20/2013 09:12 AM, Richard Biener wrote:
> On Wed, Nov 20, 2013 at 2:35 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
>> This has been bugging me since I moved it out of gimple.h to gimplify.h.
>>
>> The gimplify context structure was exposed in the header file to allow a few
>> other files to push and pop contexts off the gimplification stack.
>> Unfortunately, the struct contains a hash-table template, which means it
>> also required hash-table.h in order to even parse the header.  No file other
>> than gimplify.c actually uses what is in the structure, so exposing it seems
>> wrong.
>>
>> This patch primarily changes push_gimplify_context () and
>> pop_gimplify_context () to grab a struct gimplify_ctx off a local stack pool
>> rather than have each file's local function know about the structure and
>> allocate it on their stack.  I didn't want to use malloc/free or there would
>> be a lot of churn.  Its open to debate what the best approach is, but I
>> decided to simply allocate a bunch on a static array local to gimplify.c.
>> If it goes past the end of the local array, malloc the required structure.
>> Its pretty simplistic, but functional and doesn't required any GTY stuff or
>> other machinery.
>>
>> I also hacked up the compiler to report what the 'top' of the stack was for
>> a compilation unit. I then ran it through a bootstrap and full testsuite run
>> of all languages, and looked for the maximum number of context structs in
>> use at one time.  Normally only 1 or 2 were ever in use, but its topped out
>> at 7 during some of the openmp tests.  So my current limit of 30 will likely
>> never been reached.
>>
>> only 2 fields were ever set by anyone outside of gimplify.c, the 'into_ssa'
>> and 'allow_rhs_cond_expr' fields, and they were always set
>> immediately after pushing the context onto the stack... So I added them as
>> parameters to the push and gave them default values.
>>
>> And thats it... this patch moves the hash function and structure completely
>> within gimplify.c so no one needs to know anything about it, and removes the
>> hash-table.h include prerequisite for parsing.
>>
>> Bootstraps on x86_64-unknown-linux-gnu with no new regressions. OK for
>> mainline?
> The limit looks reasonable, but you could have used a simple linked
> list (and never free).  Also being able to pop a random context
> looks fragile ...  that is, pop_gimplify_context shouldn't have an argument.
>
>
The argument to pop_gimplify_context isn't a context pointer, its a 
gimple statement and if present calls declare_vars on the temps in the 
body...  the routine always pops the current context.  So that doesn't 
seem too fragile?

I had considered just a linked list and never freeing, but thought that 
policy might be frowned upon and trigger false positives on memory 
allocation analysis tools  :-)   I'm happy to do that tho, its quite 
trivial.


Andrew