added option to pass candidate functions#34
added option to pass candidate functions#34kuldeepmeel wants to merge 29 commits intoaig-shared-ptr-better-fixfrom
Conversation
Update checker Fixing warning Renaming I guess improved Cleanup UPdate More sane depends_on Smaller AIGs NO need No slow Adding BW and BVA-XOR option
Update Fix
Update checker Fixing warning Renaming I guess improved Cleanup UPdate More sane depends_on Smaller AIGs NO need No slow Adding BW and BVA-XOR option
|
I have also added more commits that set all the unsat defined variables as equal for unate check and also fix a segfault. I can separate into different pull requests as well. Let me know. |
|
OK, let me push a version of this to the code that is I think (more?) correct :) But I see where this is going, and indeed it's a LOT faster. I'll tag these commits with your name so it's clear I didn't come up with the idea myself :) |
|
Sorry, I am pushing only the parts related to unate_def speedup to the code :) I will review and merge the rest separately. |
|
OK, so with the fixed values added as constants, we get 2=3x speedup, and by replacing the SAT solver with CaDiCaL we get another 4-5x speedup, so 30 seconds for doing unate_def on |
|
The unate_def for sortnetsort9.AE.stepl.011.qdimacs.cnf was 416 seconds, now it's 17.88 seconds. For sdlx-fixpoint-8.qdimacs.cnf it was 4.32s now it's 4.11s So we are definitely much faseter, but the speedup is not distributed evenly. It's very nice, though, thanks for this observation. I think we can live with this speed for the moment, we don't need Oracle. |
|
I'll rebase this on the newest changes (my updates to unate_def) and then review & merge. Thanks! This will have to wait for tomorrow. |
|
For sortnetsort9.AE.stepl.012.qdimacs.cnf we went from 3000+ seconds to 38s. Actually, 38s is still a bit much, but it's a lot more reasonable. I'll think about how it could be made better. Also, the solution cache is useless. |
|
Sorry, I had pushed from my work machine but forgot to mention it here, not to consider this for the pull request. |
091121c to
860aefc
Compare
|
OK, I have imported all unate changes, made it cleaner (sampl_set is not needed, If you want to work on it, you need to pull. |
msoos
left a comment
There was a problem hiding this comment.
I added some comments. I now kinda understand where this is coming from. It's a good idea to have candicate AIGs passed. But the way it's doing it is very messy. I can try to clean this up tomorrow evening CET time, or if you wanna have a go at it, that's even better. But as-is it's messy, because it complicates a lot of logic. I'll have a hard time adding new ideas, as I'd be afraid it breaks this somehow. I'll see what I can do to make this easier on the eye/brain. I can understand it as-is, but I'm worried it'll be hard to deal with later.
| bool get_preserve_existing_defs() const { return preserve_existing_defs; } | ||
|
|
||
| private: | ||
| bool preserve_existing_defs = false; |
There was a problem hiding this comment.
This should be true by default. I don't see why it needs to be false by default?
| cnf.get_var_types(conf.verb | verbose_debug_enabled, "start do_synthesis"); | ||
| if (do_synth_bve && !cnf.synth_done()) { | ||
| if (!candidate_defs_file.empty()) { | ||
| cnf.set_preserve_existing_defs(true); |
There was a problem hiding this comment.
This should be default. When this candidate_defs_file is set, only then do we need this set to false.
Also, can't we just skip defining them completely, and use the definitions from the file? Then we wouldn't need to overwrite. I'd prefer that actually :)
There was a problem hiding this comment.
So the reason I defined these is because there can be two cases: we are trying to overwrite definitions because another pass discovered new definition: for example, backward definability can discover one definition and then when we are doing BVE, we may want to overwrite them or preserve the earlier definition. Perhaps more acute is the case when a variable is detected to be unate but we also want to eliminate it via BVE.
src/interpolant.cpp
Outdated
| auto flush_terms = [&]() { | ||
| if (!same_op_started) return; | ||
| aig = combine_balanced(std::move(same_op_terms), same_op_is_and); | ||
| same_op_terms.clear(); | ||
| same_op_started = false; | ||
| }; |
There was a problem hiding this comment.
I believe this balanced thing is part of another PR, #35. Let's fix that up, and remove this from here, so we can work on these independently. I wanna push these all in, but I'd prefer to have them reviewed/fixed independently
| } | ||
|
|
||
| void Manthan::fill_var_to_formula_with(set<uint32_t>& vars) { | ||
| void Manthan::fill_var_to_formula_with_backward(const bool include_to_define) { |
There was a problem hiding this comment.
I think the case where we have include_to_define = true should be a separate function. It now does two very different things, and I'm a bit worried it'll be hard to understand. Code duplication is OK in this case, I think .The way it is now it's quite hard to tell what's going on.
|
The I will remove the possibility of doing a mapping in |
|
Well, I didn't mean to close it to be honest, no idea how that happened. I see that we need to be able to do what this PR is doing. I just find this to be way too hacky. I'll think about it |
|
OK, I am reopening and trying to find a way to do this right. We must be able to fuzz this. |
This option can allow us to take candidate functions as input. Will be helpful to debug repair.