[cfe-dev] Patch proposal for __builtin_offsetof construct.
Douglas Gregor
dgregor at apple.com
Wed Apr 28 15:19:44 PDT 2010
On Apr 22, 2010, at 12:30 PM, Roberto Amadini wrote:
> Douglas Gregor ha scritto:
>> On Apr 16, 2010, at 3:35 AM, Roberto Amadini wrote:
>>
>>
>>> Hi,
>>>
>>> I'm the student who is trying to fix the problem mentioned in
>>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-February/007833.html .
>>>
> [...]
>>> Index: include/clang/AST/Expr.h
>>> ===================================================================
>>> --- include/clang/AST/Expr.h (revisione 100962)
>>> +++ include/clang/AST/Expr.h (copia locale)
>>> @@ -991,6 +985,78 @@
>>>
> [...]
>>> +public:
>>> + // __builtin_offsetof(type, identifier(.identifier|[expr])*)
>>> + // FIXME: Type-dependent case ignored.
>>> + struct OffsetOfNode {
>>> + SourceLocation LocStart, LocEnd;
>>> + bool isBrackets; // true if [expr], false if .ident
>>> + union {
>>> + FieldDecl *MemberDecl;
>>> + Expr *IndexExpr;
>>> + } U;
>>> + };
>>>
>>
>> You could save a word of storage per OffsetOfNode by mangling the "isBrackets" bit into the lowest bit of the FieldDecl or Expr pointer. llvm::PointerUnion<FieldDecl *, Expr *> will do this for you.
>>
>> But, see below for more comments on this representation.
>>
> In Order to have Expr*'s in contiguous memory, I replaced *IndexExpr
> field with an unsigned integer ExprIndex, which represents the index of the
> corresponding Expr* in the array of subscript expressions: in this way, I can't
> use a PointerUnion, or not?
Correct. PointerUnion won't help here. Instead, I played more fun tricks by using the lower two bits of a pointer-sized integer as a descriminator. For a FieldDecl*, we mask off those bits and cast the type; for an expression index, we shift away those bits.
>>> Index: lib/AST/StmtProfile.cpp
>>> ===================================================================
>>> --- lib/AST/StmtProfile.cpp (revisione 100962)
>>> +++ lib/AST/StmtProfile.cpp (copia locale)
>>> @@ -261,6 +261,10 @@
>>> ID.AddInteger(S->getOpcode());
>>> }
>>>
>>> +void StmtProfiler::VisitOffsetOfExpr(OffsetOfExpr *S) {
>>> + VisitExpr(S);
>>> +}
>>> +
>>>
>>
>> To profile an OffsetOfExpr, we'll need to visit the type and all of the OffsetOfNodes as well. That way, we can distinguish between different OffsetOfExprs on the same base type.
>>
>>
> Ok, but I have a doubt: VisitExpr(S) visits recursively all the sub-expressions,
> thus I think that I haven't to explicitly visit such expressions while I'm iterating on
> OffsetOfNodes array for declarations visit... Or not?
Right. VisitStmt(S) will get the subexpressions, but it won't get the field declarations.
>> Overall, this is a great start on __builtin_offsetof, thanks! I look forward to seeing a revised patch.
>>
>> - Doug
>>
> Thanks for your comments! I tried to follow your advices...
> I attached a new patch which refers to revision no. 102050.
This is great. I've committed a modified version of your patch, where I compressed the storage of OffsetOfNode a bit and added support for template instantiation and precompiled headers. Since we already had __builtin_offsetof that somewhat worked, everything had to go in at once. Here's what I ended up doing:
- Implemented the bit-mangled encoding I described above, to shrink the size of OffsetOfNode. Added accessors/constructors to OffsetOfNode, so that the representation is hidden.
- Simplified the pointer arithmetic inside OffsetOfExpr.
- Added support for templates:
- OffsetOfNode now has an "Identifier", for the case where the offsetof designates a field but we can't look up the field yet (since the record type is dependent).
- Reorganized semantic checking so that it copes with dependent types.
- Added template instantiation logic
- Added support for reading/writer OffsetOfExpr to precompiled headers
The commit is here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20100426/029847.html
Thanks for working on this!
- Doug
More information about the cfe-dev
mailing list