[cfe-commits] r42452 - in /cfe/trunk: AST/Expr.cpp Lex/IdentifierTable.cpp Parse/ParseObjc.cpp Sema/Sema.h Sema/SemaDecl.cpp Sema/SemaExpr.cpp clang.xcodeproj/project.pbxproj include/clang/AST/ASTContext.h include/clang/AST/Decl.h include/clang/AST/Expr.h include/clang/Lex/IdentifierTable.h include/clang/Lex/Preprocessor.h include/clang/Parse/Action.h include/clang/Parse/Parser.h

[Steve Naroff]

With the exception of DenseMap, Commit 42643 should address all of  
your issues below (thanks for the feedback).

I will make the DenseMap change separately.

snaroff

On Oct 3, 2007, at 10:42 PM, Chris Lattner wrote:

> On Sep 28, 2007, at 3:22 PM, Steve Naroff wrote:
>> Author: snaroff
>> Date: Fri Sep 28 17:22:11 2007
>> New Revision: 42452
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=42452&view=rev
>> Log:
>>
>> Yesterday I discovered that 78% of all selectors in "Cocoa.h" take  
>> 0/1 argument.
>>
>> This motivated implementing a devious clattner inspired solution:-)
>>
>> This approach uses a small value "Selector" class to point to an  
>> IdentifierInfo for the 0/1 case. For multi-keyword selectors, we  
>> instantiate a MultiKeywordSelector object (previously known as  
>> SelectorInfo). Now, the incremental cost for selectors is only  
>> 24,800 for Cocoa.h! This saves 156,592 bytes, or 86%!! The size  
>> reduction is also the result of getting rid of the AST slot, which  
>> was not strictly necessary (we will associate a selector with it's  
>> method using another table...most likely in Sema).
>
> Very nice Steve!  Some comments:
>
>> ===================================================================== 
>> =========
>> --- cfe/trunk/include/clang/Lex/IdentifierTable.h (original)
>> +++ cfe/trunk/include/clang/Lex/IdentifierTable.h Fri Sep 28  
>> 17:22:11 2007
>> @@ -170,28 +170,24 @@
>>    void AddKeywords(const LangOptions &LangOpts);
>>  };
>>
>> +/// MultiKeywordSelector - One of these variable length records  
>> is kept for each
>> +/// parsed selector (similar in spirit to IdentifierInfo). We use  
>> a folding set
>> +/// to unique aggregate names (keyword selectors in ObjC parlance).
>> +class MultiKeywordSelector : public llvm::FoldingSetNode {
>
> I don't think there is any reason to export the definition of  
> MultiKeywordSelector in the header file.  Can you make it just  
> forward-declared here and suck the definition/implementation into  
> IdentifierTable.cpp?  Clients should never interact with  
> MultiKeywordSelector, they should only interact with Selector.
>
> Ahh, I see you added this later:
> /// The only reason it appears in this header is FoldingSet needs  
> to see it:-(
>
> Presumably this is because Preprocessor contains:
>   /// Selectors - This table contains all the selectors in the  
> program. Unlike
>   /// IdentifierTable above, this table *isn't* populated by the  
> preprocessor.
>   /// It is declared/instantiated here because it's role/lifetime is
>   /// conceptually similar the IdentifierTable. In addition, the  
> current control
>   /// flow (in clang::ParseAST()), make it convenient to put here.
>   /// FIXME: Make sure the lifetime of Identifiers/Selectors  
> *isn't* tied to
>   /// the lifetime fo the preprocessor.
>   llvm::FoldingSet<MultiKeywordSelector> Selectors;
>
> This declaration is unfortunate for two reasons:
> 1. it requires MultiKeywordSelector to be exported out of  
> IdentifierTable.cpp, and
> 2. it requires preprocessor and IdentifierTable.h to #include  
> FoldingSet.h
>
> Instead of doing this, I'd suggest adding the following to  
> IdentifierTable.h:
>
> class SelectorUniquingTable {
>   void *Impl;  // Actually a FoldingSet<MultiKeywordSelector>*
>   SelectorUniquingTable(const SelectorUniquingTable&); // DISABLED:  
> DO NOT IMPLEMENT
>   void operator=(const SelectorUniquingTable&); // DISABLED: DO NOT  
> IMPLEMENT
> public:
>   SelectorUniquingTable();  // new's the foldingset
>   ~SelectorUniquingTable();  // delete's the foldingset
>
>   Selector getSelector(unsigned nKeys, IdentifierInfo **IIV);
>   Selector getUnarySelector(IdentifierInfo *ID);
>   Selector getNullarySelector(IdentifierInfo *ID);
> };
>
> With this definition, Parser can contain an instance of  
> SelectorUniquingTable instead of a folding set, getting rid of the  
> #include of FoldingSet.h and allowing MultiKeywordSelector to  
> become private to the .cpp file.
>
> Another benefit is that stuff like this (from Selector and  
> MultiKeywordSelector):
>> +  friend class Parser; // only the Parser can create these.
>
> can now befriend the SelectorUniquingTable class instead, fixing a  
> layering problem (because ideally IdentifierTable will sink into  
> libbasic).
>
>
>> +  IdentifierInfo *getIdentifierInfoForSlot(unsigned i) {
>> +    assert((i < NumArgs) && "getIdentifierInfoForSlot(): illegal  
>> index");
>> +    return keyword_begin()[i];
>> +  }
>
> This method should be const.
>
> +  friend class llvm::FoldingSet<MultiKeywordSelector>;
>
> This can go away when it is in the .cpp file, because all the  
> access can be public.
>
>> +class Selector {
>
> The selector class needs a comment above it.
>
>> +  // Derive the full selector name, placing the result into  
>> methodBuffer.
>> +  // As a convenience, a pointer to the first character is returned.
>> +  // Example usage: llvm::SmallString<128> mbuf; Selector->getName 
>> (mbuf);
>> +  char *getName(llvm::SmallVectorImpl<char> &methodBuffer);
>
> Why is this implemented in ParseObjc.cpp?  It seems like it should  
> be in IdentifierTable.cpp.
>
>>
>> Modified: cfe/trunk/Sema/SemaDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/Sema/ 
>> SemaDecl.cpp?rev=42452&r1=42451&r2=42452&view=diff
>>
>> ===================================================================== 
>> =========
>> --- cfe/trunk/Sema/SemaDecl.cpp (original)
>> +++ cfe/trunk/Sema/SemaDecl.cpp Fri Sep 28 17:22:11 2007
>>
>> @@ -1206,35 +1206,35 @@
>>  static void ImplMethodsVsClassMethods(Sema* objSema,
>>  				      ObjcImplementationDecl* IMPDecl,
>>                                        ObjcInterfaceDecl* IDecl) {
>> -  llvm::DenseMap<const SelectorInfo*, char> InsMap;
>> +  llvm::DenseMap<void *, char> InsMap;
>
> Since this code was written, a new llvm::DenseSet was added.  You  
> should be able to switch this to use DenseSet, which makes it more  
> obvious what you're doing here.
>
> Also, instead of using .getAsOpaquePtr(), I think it would make  
> sense to teach the hash table how to handle Selectors.  It seems  
> useful to other people and can be added easily to IdentifierInfo.h.
>
>> ===================================================================== 
>> =========
>> --- cfe/trunk/include/clang/AST/Expr.h (original)
>> +++ cfe/trunk/include/clang/AST/Expr.h Fri Sep 28 17:22:11 2007
>> @@ -1100,12 +1101,17 @@
>>    Expr *getReceiver() { return SubExprs[RECEIVER]; }
>>
>>    /// getNumArgs - Return the number of actual arguments to this  
>> call.
>> +  unsigned getNumArgs() const { return SelName.getNumArgs(); }
>> +
>> +/// getArg - Return the specified argument.
>> +  Expr *getArg(unsigned Arg) {
>> +    assert(Arg < SelName.getNumArgs() && "Arg access out of  
>> range!");
>> +    return SubExprs[Arg+ARGS_START];
>> +  }
>
> Funky comment indentation.
>
> Overall, huge leap forward!
>
> -Chris
>