[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]

On Oct 5, 2007, at 11:49 AM, Fariborz Jahanian wrote:

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

Thanks!

snaroff

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