Re: [cxx-conversion] gimplify_ctx::temp_htab hash table

[Lawrence Crowl <crowl at googlers dot com>]

On 12/4/12, Diego Novillo <dnovillo@google.com> wrote:
> On Tue, Dec 4, 2012 at 4:23 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Dec 3, 2012 at 9:14 PM, Lawrence Crowl <crowl@googlers.com>
>> wrote:
>>> On 12/3/12, Diego Novillo <dnovillo@google.com> wrote:
>>>> On 2012-12-01 20:44 , Lawrence Crowl wrote:
>>>>> Index: gcc/gimple-fold.c
>>>>> ===================================================================
>>>>> --- gcc/gimple-fold.c        (revision 193902)
>>>>> +++ gcc/gimple-fold.c        (working copy)
>>>>> @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3.
>>>>>   #include "tree-ssa-propagate.h"
>>>>>   #include "target.h"
>>>>>   #include "gimple-fold.h"
>>>>> +#include "gimplify-ctx.h"
>>>>>
>>>>>   /* Return true when DECL can be referenced from current unit.
>>>>>      FROM_DECL (if non-null) specify constructor of variable DECL was
>>>>> taken from.
>>>>> Index: gcc/tree-mudflap.c
>>>>> ===================================================================
>>>>> --- gcc/tree-mudflap.c       (revision 193902)
>>>>> +++ gcc/tree-mudflap.c       (working copy)
>>>>> @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.
>>>>>   #include "ggc.h"
>>>>>   #include "cgraph.h"
>>>>>   #include "gimple.h"
>>>>> +#include "gimplify-ctx.h"
>>>>>
>>>>>   extern void add_bb_to_loop (basic_block, struct loop *);
>>>>>
>>>>> Index: gcc/tree-inline.c
>>>>> ===================================================================
>>>>> --- gcc/tree-inline.c        (revision 193902)
>>>>> +++ gcc/tree-inline.c        (working copy)
>>>>> @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3.
>>>>>   #include "value-prof.h"
>>>>>   #include "tree-pass.h"
>>>>>   #include "target.h"
>>>>> +#include "gimplify-ctx.h"
>>>>
>>>> I don't follow.  It seems that factoring into gimplify-ctx.h does
>>>> not actually buy much.  The files using it are just including
>>>> *another* file.  Whereas previously, they were getting that
>>>> content from gimple.h.
>>>>
>>>> Unless we can stop including gimple.h from these files, I don't
>>>> see a lot of gain in this factoring.  Am I missing something?
>>>
>>> There at least 70 files that include gimple.h, and only 5 that need
>>> gimple-ctx.h.  By splitting it out, at least 65 files will not need
>>> to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct,
>>> the gimplify_hasher template struct, and may not need to include
>>> hash-table.h.
>>>
>>> It's all about avoiding superfluous compilation in other files.
>>
>> But it's backward - gimple.h is the core file as it provides GIMPLE.
>> The gimplifier and its context certainly requires to know about
>> GIMPLE.
>>
>> Btw, if we still want to split it I'd rather have gimplify.h (and also
>> put all exports from gimplify.c there, not only gimplify_ctx).  Still
>> I don't think it would buy us much.
>
> We talked offline about this with Lawrence.  I was not too convinced
> about adding this new header file, it seemed forced but I was
> struggling with a better alternative.  Perhaps gimplify.h is a better
> way, but I'm not sure either.
>
> We decided that since it's in the branch, it may not matter much for
> now.  However, another thing occurred to me in the meantime: merges.
> The new file will probably cause merge problems.  If we are not
> completely convinced that it's a good change, we are making our life
> harder for no gain.
>
> Lawrence, you're going to hate me, but would you mind just leaving the
> ctx structure in gimple.h for now?  Sorry!  I think it's going to be
> easier to just leave it there for now until we come up with a good
> split for the constructs in that file.

Okay, I'll do that.

-- 
Lawrence Crowl