Skip to content
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.

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

Multiple RHS Support #367

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

zhengyang92
Copy link
Contributor

@zhengyang92 zhengyang92 commented Oct 10, 2018

This patch changes the type of the RHS argument of ExhaustiveSynthesis::synthesize() and Solver::infer() from a Inst pointer to a vector of Inst pointers.

Exhaustive Synthesis takes a constant "MaxRHS". This constant is set to 1 for performance. Exhaustive Synthesis generates at most MaxRHS number of RHSs. The original Souper test suite on my machine runs for 282.54s while with MaxRHS set to 3, the test suite runs for 454.83s.

Call sites of infer() function takes the result vector of RHS and extract the first RHS, this preserves the behaviors of these call sites.

@zhengyang92
Copy link
Contributor Author

zhengyang92 commented Oct 10, 2018

Note: no support for multiple RHSs dump in souper-check or redis cache for now.

@@ -103,7 +104,7 @@ class BaseSolver : public Solver {
if (EC)
return EC;
if (!IsSat) {
RHS = I;
RHSs.emplace_back(I);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to return here, or try to find more RHSs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are both constant synthesis, why we wants to collect multiple RHSs if a constant fit the LHS

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it depends on why we want multiple RHSs. for manasij's purposes a constant is always good, but for building a cost model I'd like as many alternatives as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping me if you need this.

@@ -142,7 +143,7 @@ class BaseSolver : public Solver {
if (EC)
return EC;
if (!IsSat) {
RHS = Const;
RHSs.emplace_back(Const);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -214,18 +215,21 @@ class BaseSolver : public Solver {
if(SMTSolver->supportsModels()) {
if (EnableExhaustiveSynthesis) {
ExhaustiveSynthesis ES;
EC = ES.synthesize(SMTSolver.get(), BPCs, PCs, LHS, RHS, IC, Timeout);
if (EC || RHS)
EC = ES.synthesize(SMTSolver.get(), BPCs, PCs, LHS, RHSs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you intend to change this line? was it too long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RHS is changed to RHSs, and it is too long so I wrapped.

if (!EC && RHS) {
RHSStr = GetReplacementRHSString(RHS, Context);
if (!EC && !RHSs.empty()) {
RHSStr = GetReplacementRHSString(RHSs.front(), Context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems weird to take the first element here, is this a placeholder for something different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR does not support cache dumping yet, so only print the first RHS in the list. I'll add support for cache dumping soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say don't do the caching part of this yet, until we're sure we want it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, should be possible to prototype without caching.

@@ -438,6 +439,7 @@ ExhaustiveSynthesis::synthesize(SMTLIBSolver *SMTSolver,
std::vector<Inst *> Vars;
findVars(LHS, Vars);

unsigned ValidRHS = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you want a separate variable for this? doesn't it work to inspect the length of the vector?

@regehr
Copy link
Collaborator

regehr commented Oct 10, 2018

overall I'm not sure about this patch
I do want this for cost model purposes, but it seems to add complexity and the calls to RHS.first() seem displeasing. I might merge this into a branch of my own for now, but not merge into master

@zhengyang92
Copy link
Contributor Author

Sure, I'll remove the unnecessary validRHS variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants