Page MenuHomePhabricator

[Concepts] Constraint Satisfaction Caching
ClosedPublic

Authored by saar.raz on Jan 10 2020, 6:56 PM.

Details

Summary

Add a simple cache for constraint satisfaction results. This results in a
major speedup (cjdb measured x2 faster than gcc with this caching, compared
to being 'unbearably slow' without caching) and is conformant with gcc's
implementation.

Event Timeline

saar.raz created this revision.Jan 10 2020, 6:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 6:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith accepted this revision.Jan 21 2020, 1:09 PM

Mechanically, this looks fine.

There's an ongoing discussion in the committee as to whether this kind of caching is permissible. But if this is necessary for acceptable performance, let's take this for now and figure out how to invalidate the cache as necessary later. Perhaps a -f flag to turn this off would be useful, so we can demonstrate and measure the performance delta.

This revision is now accepted and ready to land.Jan 21 2020, 1:09 PM

Mechanically, this looks fine.

There's an ongoing discussion in the committee as to whether this kind of caching is permissible. But if this is necessary for acceptable performance, let's take this for now and figure out how to invalidate the cache as necessary later. Perhaps a -f flag to turn this off would be useful, so we can demonstrate and measure the performance delta.

If there were a flag to turn this on and off, I would be happy to provide numbers for a real code base so we can understand how important of an optimization this is.

This revision was automatically updated to reflect the committed changes.
rtrieu added a subscriber: rtrieu.Jan 21 2020, 5:47 PM
rtrieu added inline comments.
clang/lib/AST/ASTConcept.cpp
17

This causes a circular dependency between AST and Sema. It looks like you are including this header to get access to some classes, but you should include the direct header instead. These are the headers for the classes you are using in ConstraintSatisfaction::Profile:

NamedDecl -> clang/AST/Decl.h
TemplateArgument -> clang/AST/TemplateBase.h
ArrayRef -> llvm/ADT/ArrayRef.h
FoldingSetNodeID -> llvm/ADT/FoldingSet.h

Please update the header includes to resolve the circular dependency.

It looks like the concept changes broke the debugger (quite spectacularly actually):

http://lab.llvm.org:8011/builders/lldb-x86_64-debian/builds/3356
http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/12872
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/6407/

I'm rebuilding locally to see if I can reproduce

Most likely because of https://reviews.llvm.org/D65042 rather than this change, I see you made changes to the AST importer.