On Wed, Nov 20, 2013 at 3:34 PM, Andrew MacLeod <amacl...@redhat.com> wrote: > On 11/20/2013 09:12 AM, Richard Biener wrote: >> >> On Wed, Nov 20, 2013 at 2:35 PM, Andrew MacLeod <amacl...@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?
Oh... I see. > 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. ;) Richard. > > Andrew > >
