-
Notifications
You must be signed in to change notification settings - Fork 170
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
Floating point cost values with some heuristics for ExhaustiveSynthesis #399
base: main
Are you sure you want to change the base?
Conversation
@@ -50,6 +50,8 @@ namespace { | |||
static cl::opt<bool> EnableBigQuery("souper-exhaustive-synthesis-enable-big-query", | |||
cl::desc("Enable big query in exhaustive synthesis (default=false)"), | |||
cl::init(false)); | |||
static cl::opt<std::string> CostModel("souper-cost-model", cl::desc("Cost Model"), | |||
cl::init("default")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the description you should list the possible choices
using CostFunctionType = std::function<float(Inst *, bool)>; | ||
|
||
float defaultCostFunction(Inst* I, bool IgnoreDepsWithExternalUses) { | ||
return souper::cost(I, IgnoreDepsWithExternalUses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well move the code over here, instead of calling out to somewhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old synthesis still relies on this.
And having it support new cost models is tricky.
(eg; #395)
@@ -86,6 +88,60 @@ namespace { | |||
// experiment with synthesizing at reduced bitwidth, then expanding the result | |||
// aggressively avoid calling into the solver | |||
|
|||
using CostFunctionType = std::function<float(Inst *, bool)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use double instead of float everywhere
return souper::cost(I, IgnoreDepsWithExternalUses); | ||
} | ||
|
||
// TODO : Derive these randomly to maximize infer rate with test-infer.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "derive these randomly" mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pick a plausible cost range and a sample frequency for each instruction.
For all possible cost combinations, run test-infer.sh on the solver/alive directory and pick the combinations with the best infer success rate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't explain it here, explain it in the code
bool IgnoreDepsWithExternalUses) { | ||
if (!Visited.insert(I).second) | ||
return 0; | ||
if (IgnoreDepsWithExternalUses && I != Root && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should leave a comment explaining the rationale for counting things with external uses are zero cost
int &TooExpensive) { | ||
if (souper::cost(RHS) < MaxCost) | ||
void addGuess(Inst *RHS, float MaxCost, std::vector<Inst *> &Guesses, | ||
float &TooExpensive, CostFunctionType &Cost) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than passing the cost function around a lot, investigate whether there's a better place to stash this pointer, for example in one of the context objects
if (souper::cost(RHS) < MaxCost) | ||
void addGuess(Inst *RHS, float MaxCost, std::vector<Inst *> &Guesses, | ||
float &TooExpensive, CostFunctionType &Cost) { | ||
if (Cost(RHS, false) < MaxCost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when passing a boolean constant, follow the convention of including the name of the flag as a comment
CostFunctionType Cost; | ||
if (CostModel == "default") { | ||
Cost = defaultCostFunction; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the else clause needs to be an error
I agree that we want pluggable cost functions, but do you need to add a new one right now? it is very similar to the existing one, so maybe the existing one simply wants to be replaced (while also leaving the pluggable infrastructure so that we can plug in more sophisticated stuff later)? |
@@ -0,0 +1,11 @@ | |||
; REQUIRES: solver, solver-model | |||
|
|||
; RUN: %souper-check %solver -reinfer-rhs -souper-exhaustive-synthesis -souper-exhaustive-synthesis-num-instructions=1 -souper-cost-model=heuristics %s > %t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a great cost model test, since many different cost models would allow this optimization to go.
please try to find a place in our unit tests where you can test the cost model directly, including whether it properly deals with the costs of external uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default one (without ignoring cost) does not infer this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a good test case and wants to be improved
I think it makes sense to keep the existing one because the CEGIS synthesis needs it. |
it's fine to leave the old one if you want, but make the other changes |
Implements some infrastructure to support richer cost models.