On Sep 9, 2011, at 11:05 AM, Douglas Gregor wrote:
>
> On Sep 9, 2011, at 10:40 AM, Argyrios Kyrtzidis wrote:
>
>> On Sep 9, 2011, at 10:30 AM, Douglas Gregor wrote:
>>
>>>
>>> On Sep 9, 2011, at 10:28 AM, Argyrios Kyrtzidis wrote:
>>>
>>>> On Sep 9, 2011, at 9:55 AM, Douglas Gregor wrote:
>>>>
>>>>>
>>>>> On Sep 9, 2011, at 9:44 AM, Argyrios Kyrtzidis wrote:
>>>>>
>>>>>>
>>>>>> On Sep 9, 2011, at 9:12 AM, Douglas Gregor wrote:
>>>>>>
>>>>>>>
>>>>>>> On Sep 9, 2011, at 9:05 AM, Argyrios Kyrtzidis wrote:
>>>>>>>
>>>>>>>> On Sep 9, 2011, at 8:15 AM, Douglas Gregor wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sep 8, 2011, at 11:44 PM, Argyrios Kyrtzidis wrote:
>>>>>>>>>
>>>>>>>>>> Author: akirtzidis
>>>>>>>>>> Date: Fri Sep 9 01:44:17 2011
>>>>>>>>>> New Revision: 139358
>>>>>>>>>>
>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=139358&view=rev
>>>>>>>>>> Log:
>>>>>>>>>> [PCH] When loading the decls linked to an identifier, also make them
>>>>>>>>>> visible
>>>>>>>>>> in the translation unit.
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>> cfe/trunk/lib/Serialization/ASTReader.cpp
>>>>>>>>>>
>>>>>>>>>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
>>>>>>>>>> URL:
>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=139358&r1=139357&r2=139358&view=diff
>>>>>>>>>> ==============================================================================
>>>>>>>>>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
>>>>>>>>>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Sep 9 01:44:17
>>>>>>>>>> 2011
>>>>>>>>>> @@ -4841,8 +4841,16 @@
>>>>>>>>>> return;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> + ASTContext &Ctx = *getContext();
>>>>>>>>>> for (unsigned I = 0, N = DeclIDs.size(); I != N; ++I) {
>>>>>>>>>> NamedDecl *D = cast<NamedDecl>(GetDecl(DeclIDs[I]));
>>>>>>>>>> +
>>>>>>>>>> + // In C++, translation unit's visible decls map gets emitted in
>>>>>>>>>> the AST
>>>>>>>>>> + // file, but not on C; make the decl visible so it can be
>>>>>>>>>> looked up.
>>>>>>>>>> + if (!Ctx.getLangOptions().CPlusPlus)
>>>>>>>>>> + SetExternalVisibleDeclsForName(Ctx.getTranslationUnitDecl(),
>>>>>>>>>> + DeclarationName(II), D);
>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a bit unfortunate. Creating the lookup table in the
>>>>>>>>> translation unit forces the AST to continually maintain that lookup
>>>>>>>>> table (e.g., adding entries to it every time a visible declaration is
>>>>>>>>> added to the translation unit), which is a cost we'd like to avoid in
>>>>>>>>> C. I presume that you only made this change to support the new lookup
>>>>>>>>> used in r139359? I have an alternative suggestion for that...
>>>>>>>>
>>>>>>>> But adding entries always updated the lookup table, or the lookup in
>>>>>>>> r139359 would not work in non-PCH mode, right ?
>>>>>>>> In general I'd find it bad for the AST usefulness if you could not do
>>>>>>>> a lookup in translation unit unless it is on C++.
>>>>>>>
>>>>>>>
>>>>>>> It's more subtle than that. The lookup table for a DeclContext is
>>>>>>> constructed lazily, when one performs the first lookup, by walking the
>>>>>>> declarations lexically in that context and populating the table. Once a
>>>>>>> table is there, it is maintained as declarations are added to the
>>>>>>> DeclContext.
>>>>>>
>>>>>> I see the error of my ways now. Note however that, outside of this fix,
>>>>>> it's very unfortunate that lookup does not work on a loaded C AST.
>>>>>
>>>>>
>>>>> I think it should work, but it's super-inefficient because it will
>>>>> deserialize the whole translation unit.
>>>>
>>>> Once a declaration is loaded through the identifier it can be found in
>>>> lexical decls linked list right ? It's only of matter then to figure out
>>>> that if no result is returned from looking up at the external ast source,
>>>> it doesn't mean that no decl with that name exists, but that the source
>>>> didn't store a map; in which case we recreate the map with only the
>>>> lexical decls that were already loaded (and subsequent
>>>> loaded-through-identifier decls will get added to the existing map)
>>>
>>>
>>> Ahhh, you are correct. Which means this is already broken :(
>>>
>>> Perhaps we should just take the performance hit when building an AST file
>>> for a C translation unit, generating the lookup table for the translation
>>> unit as AST writing time so we can serialize it.
>>
>> Rough estimate: ObjC PCH for Cocoa.h is -20% compared to the ObjC++ PCH, is
>> this acceptable hit ?
>
>
> ObjC++ includes a bunch of extra stuff, so that will over-estimate the cost…
> I went ahead and measured the actual cost of doing this. It's an 8% increase
> in PCH size, which I think is unacceptable.
>
> Looking back at DeclContext::buildLookup, I don't actually see why it would
> be broken. It walks the lexical context using decls_begin()/decls_end(),
> which should load all of the lexical declarations on-demand from the PCH
> file. Something is fishy here.
buildLookup is not getting called if there is an external ast source present,
looking up is delegated to the source; see DeclContext::lookup and the "if
(hasExternalVisibleStorage()) {" check.
>
> - Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
