This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rGaf29591650c4: [AST] Reduce the size of
TemplateArgumentLocInfo. (authored by hokein).
Changed prior to comm
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Forgot to mention, 3% memory saving is huge, way more than I expected (was
mostly just hoping for no regression).
Nice work!
Comment at: clang/include/clang/AST/Templa
hokein added inline comments.
Comment at: clang/include/clang/AST/Expr.h:963
};
+static_assert(llvm::PointerLikeTypeTraits::SpecializationForExpr,
+ "Specialization in TemplateBase.h must be seen here");
sammccall wrote:
> I think this is it:
> ```
hokein updated this revision to Diff 293094.
hokein marked 2 inline comments as done.
hokein added a comment.
address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87080/new/
https://reviews.llvm.org/D87080
Files:
clang/include/
sammccall added inline comments.
Comment at: clang/include/clang/AST/Expr.h:963
};
+static_assert(llvm::PointerLikeTypeTraits::SpecializationForExpr,
+ "Specialization in TemplateBase.h must be seen here");
I think this is it:
```
// PointerLikeType
rsmith added inline comments.
Comment at: clang/include/clang/AST/TemplateBase.h:421
-public:
- constexpr TemplateArgumentLocInfo() : Template({nullptr, nullptr, 0, 0}) {}
-
- TemplateArgumentLocInfo(TypeSourceInfo *TInfo) : Declarator(TInfo) {}
+ T *getTemplate() const {
+
hokein added a comment.
> Would it be possible to compile some big file in LLVM (probably doesn't
> matter much which, Sema something?) and observe if there's a significant
> change in overall ASTContext size?
~3% saving (measuring the `ASTContext::.getASTAllocatedMemory`)
| | Bef
hokein updated this revision to Diff 293029.
hokein marked an inline comment as done.
hokein added a comment.
address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87080/new/
https://reviews.llvm.org/D87080
Files:
clang/include/
sammccall added a comment.
Looks good apart from a minor technical issue with the traits.
Would it be possible to compile some big file in LLVM (probably doesn't matter
much which, Sema something?) and observe if there's a significant change in
overall ASTContext size?
Comme
hokein added inline comments.
Comment at: clang/include/clang/AST/TemplateBase.h:429
+ auto *T = getTemplate();
+ T->Ctx->Deallocate(T);
+}
sammccall wrote:
> sammccall wrote:
> > this is a no-op, and thus not worth stashing a pointer to Ctx for!
>
hokein updated this revision to Diff 290188.
hokein added a comment.
format
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87080/new/
https://reviews.llvm.org/D87080
Files:
clang/include/clang/AST/TemplateBase.h
clang/lib/AST/ASTImporter.cpp
hokein updated this revision to Diff 290187.
hokein marked 2 inline comments as done.
hokein added a comment.
address review comments:
- use PointerUnion
- keep the TemplateArgumentLocInfo trivial, allocate from allocator, no
deallocation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE L
sammccall added a comment.
Nit on the commit message: I think this is TemplateArgumentLoc 48 -> 32 bytes,
and DynTypedNode 56 -> 40 bytes.
Comment at: clang/include/clang/AST/TemplateBase.h:415
-public:
- constexpr TemplateArgumentLocInfo() : Template({nullptr, nullptr, 0,
hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a subscriber: martong.
Herald added a reviewer: shafik.
Herald added a project: clang.
hokein requested review of this revision.
heap-allocate the Template kind, this would reduce AST memory usage
- TemplateArgumentLoc
14 matches
Mail list logo
