Page MenuHomePhabricator

Shrink DynTypedNode by one pointer from 40 to 32 bytes (on x86_64).
ClosedPublic

Authored by bkramer on Oct 21 2015, 7:51 AM.

Details

Summary

The MemoizationData cache was introduced to avoid a series of enum
compares at the cost of making DynTypedNode bigger. This change reverts
to using an enum compare but instead of building a chain of comparison
the enum values are reordered so the check can be performed with a
simple greater than. The alternative would be to steal a bit from the
enum but I think that's a more complex solution and not really needed
here.

I tried this on several large .cpp files with clang-tidy and didn't
notice any performance difference. The test change is due to matchers
being sorted by their node kind.

Event Timeline

bkramer updated this revision to Diff 38013.Oct 21 2015, 7:51 AM
bkramer retitled this revision from to Shrink DynTypedNode by one pointer from 40 to 32 bytes (on x86_64)..
bkramer updated this object.
bkramer added a reviewer: sbenza.
bkramer added a subscriber: cfe-commits.
sbenza added inline comments.Oct 21 2015, 8:45 AM
include/clang/AST/ASTTypeTraits.h
250 ↗(On Diff #38023)

I'm not sure about this reinterpret_cast.
We are not storing void*. We are storing 'const Node*'.
I think this way is UB.

Maybe we should modify the union and the create/getUnchecked methods to use 'const void*' as storage instead.
Instead of:

new (Result.Storage.buffer) const BaseT * (&Node);
...
return *cast<T>(*reinterpret_cast<BaseT *const *>(Storage));

we should do

new (Result.Storage.buffer) const void * (static_cast<const BaseT*>(&Node));
...
return *cast<T>(static_cast<const BaseT*>(*reinterpret_cast<const void *const *>(Storage)));
bkramer updated this revision to Diff 38023.Oct 21 2015, 9:07 AM

Update casts to avoid potential undefined behavior. Now every pointer goes through const void*.

sbenza accepted this revision.Oct 21 2015, 9:19 AM
sbenza edited edge metadata.
sbenza added inline comments.
include/clang/AST/ASTTypeTraits.h
300 ↗(On Diff #38023)

maybe call getUnchecked() to avoid duplication?
Now it is getting complicated.

This revision is now accepted and ready to land.Oct 21 2015, 9:19 AM
This revision was automatically updated to reflect the committed changes.