Skip to content

[const evaluator] Parameterize allocation of symbolic values in the constant interpreter. #23456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 11, 2019
Merged

[const evaluator] Parameterize allocation of symbolic values in the constant interpreter. #23456

merged 1 commit into from
Apr 11, 2019

Conversation

ravikandhadai
Copy link
Contributor

Based on this feature, change to a short-lived bump allocator for storing symbolic values in the pass that checks #assert.

@ravikandhadai
Copy link
Contributor Author

@marcrasi Based on the discussion here: #21869, I have changed the constant interpreter and symbolic value factories to use a callback (provided by the client) to perform allocation instead of the AST Context. This enables using a short-lived bump allocator especially for checking pound_asserts, at the same time enables cloning the symbolic values to a longer-lived allocation (and also allows using ASTContext.allocate, if needed).

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

devincoughlin
devincoughlin approved these changes Apr 4, 2019
View reviewed changes
Copy link
Contributor

@devincoughlin devincoughlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Some comments inline.

include/swift/SIL/SILConstants.h Outdated
@@ -177,6 +177,8 @@ class SymbolicValue {
} auxInfo;

public:
using Allocator = std::function<void *(unsigned long, unsigned)>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to document what the first and second parameters mean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: could you get away with llvm::function_ref here rather than std::function? Given that the bump pointer allocator is stack allocated in EmitDFDiagnostics::run() , using the allocator anywhere after that will be a problem already -- so we shouldn't pay the price of std::function.

@ravikandhadai ravikandhadai force-pushed the constexpr-local-allocation branch from ac27102 to b5bc22a Compare April 8, 2019 21:51
@ravikandhadai
Copy link
Contributor Author

@devincoughlin @marcrasi I have changed the design from using closures to using an explicit abstract class: SymbolicValueAllocator, which exposes interfaces and utility functions for performing allocation. The storage for the allocated objects will be managed by derived classes. For instance, the derived class: SymbolicValueBumpAllocator uses llvm::BumpPtrAllocator and is defined in SILConstants.h. It is straightforward to add another derived class the uses ASTContext to implement the allocation methods. This design makes copying and passing of the allocator cheap and also makes reasoning about lifetime easier (compared to llvm::function_ref).

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2019

Build failed
Swift Test Linux Platform
Git Sha - ac2710278a96d5625d7da29d80a11deb892b5341

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test Linux Platform

@ravikandhadai ravikandhadai force-pushed the constexpr-local-allocation branch from b5bc22a to 8906a1f Compare April 9, 2019 20:56
@ravikandhadai
[const evaluator] Parameterize allocation of symbolic values in the
bb7363d 
constant interpreter. Based on this, change to a short-lived bump
allocator for storing symbolic values in the pass that checks #assert.
@ravikandhadai ravikandhadai force-pushed the constexpr-local-allocation branch from 8906a1f to bb7363d Compare April 9, 2019 20:56
@ravikandhadai
Copy link
Contributor Author

Fixed an obsolete comment.

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 9, 2019

Build failed
Swift Test Linux Platform
Git Sha - b5bc22a2d643857fab9c4caf31dcb98e52eef691

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

2 similar comments
@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

@swift-ci
Copy link
Contributor

swift-ci commented Apr 9, 2019

Build failed
Swift Test OS X Platform
Git Sha - b5bc22a2d643857fab9c4caf31dcb98e52eef691

@ravikandhadai
Copy link
Contributor Author

@swift-ci Please test macOS Platform

@ravikandhadai
Copy link
Contributor Author

@marcrasi Just wondering if this new design looks okay to you.

@marcrasi
Copy link

Yes, LGTM!

@ravikandhadai ravikandhadai merged commit 28efe03 into swiftlang:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devincoughlin devincoughlin devincoughlin approved these changes

@marcrasi marcrasi marcrasi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ravikandhadai @swift-ci @marcrasi @devincoughlin