[cfe-dev] Patch for ContextDecl

[Steve Naroff]

Chris,

 From my perspective, both techniques below are tedious (and disclose  
implementation details that could otherwise be hidden).

    if (II) {
// AST-based linked list...
-    New->setNext(II->getFETokenInfo<ScopedDecl>());
-    II->setFETokenInfo(New);
// Sema-based map...
+    IdDeclInfos[II].PushShadowed(New);
// Why isn't the following sufficient? Simple, no implementation  
details disclosed...
      S->AddDeclForIdentifier(New, II);
    }

// ...we could then change IdentifierInfo to have an API similar to  
Argiris's IdDeclInfo.

llvm::SmallVector<void *, 2> FETokenInfos;
/// Add a decl in the scope chain
void PushShadowed(void *D) { FETokenInfos.push_back(D); }
void PopShadowed() { FETokenInfos.pop_back(); }

This should...

- Allow us to remove the "Next" from ScopedDecl (which cleans up the  
AST node).
- Remove the overhead associated with the map.
- Simplifies the API (no need to explicitly manages chains or  
maps...). Keeps the low level details at a lower level...

Does this make sense? Am I missing something? (I've had a few glasses  
of wine and may be over simplifying:-)

snaroff

On Apr 6, 2008, at 3:16 PM, Chris Lattner wrote:

>
> On Apr 6, 2008, at 1:32 PM, Argiris Kirtzidis wrote:
>
>> Chris Lattner wrote:
>>> What is actually going on here is that getNext is horribly
>>> overloaded for scope decls.  When parsing is happening, the "next"
>>> pointer is used to represent the scope chains hanging off the
>>> identifier info.  This keeps track of shadowing.  However, after
>>> parsing is done, these fields are dead, so they are "repurposed" to
>>> be the linked list of decls within a context (e.g. functiondecl).
>>>
>>> A truly elegant solution would be to get rid of parsing-specific
>>> stuff from the AST, but I'm not sure how to do this while retaining
>>> efficiency at parse time for walking and maintaining the shadow
>>> stacks.
>> That's a good idea. A suggestion for this is the attached patch.
>
> Wow, this is an incredibly clever way to implement this.  I like the
> idea in principle.
>
> However, it appears to be a very consistent slowdown for both carbon.h
> (a large C header) and Cocoa.h (a large objc header) with
> ENABLE_OPTIMIZED=1 DISABLE_ASSERTIONS=1:
>
> Before:
>
> $ time ./clang INPUTS/carbon_h.c
> 0.180u 0.058s 0:00.24 95.8%	0+0k 0+0io 0pf+0w
> $ time ./clang INPUTS/Cocoa_h.m
> 0.197u 0.066s 0:00.26 96.1%	0+0k 0+0io 0pf+0w
>
> After:
>
> $ time ~/llvm/Release-Asserts/bin/clang INPUTS/carbon_h.c
> 0.191u 0.061s 0:00.25 100.0%	0+0k 0+0io 0pf+0w
> $ time ~/llvm/Release-Asserts/bin/clang INPUTS/Cocoa_h.m
> 0.205u 0.069s 0:00.28 92.8%	0+0k 0+0io 0pf+0w
>
> That is a 6.1% slowdown on carbon.h and 4% slowdown on Cocoa.h.   The
> number on this machine are very stable, three consecutive runs all
> produced the same timings.  While I really would like to have the
> cleanup, I don't think the cost is worth it.  When coming from a .i
> file (to reduce syscall/preprocessor overhead) the slowdown is 8.4%
> for carbon.h (0.119s -> 0.129s).
>
> What do you think?  If you want, I can provide you with a .i file of
> cocoa.h or carbon.h.
>
> -Chris
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev