[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
fjahanian at apple.com
Fri Oct 5 11:49:54 PDT 2007
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
More information about the cfe-commits
mailing list