[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

[Fariborz Jahanian]

On Oct 5, 2007, at 11:46 AM, Steve Naroff wrote:

> 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.

I have already made the change from DenseMap to DenseSet .

- fariborz

>
>
> 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
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits