Refactor operand scheduling for binary ops. #656
Conversation
It uses a `Vec<Operand>` where a `Vec<ValueOrAlias>` is simpler.
Take the special case handling and move it into a tactic for the regular solver.
| ); | ||
|
|
||
| let num_actions = builder.take().len(); | ||
| assert!(builder.take().len() <= max_actions); |
There was a problem hiding this comment.
builder.take() is called twice.
There was a problem hiding this comment.
Ooh, nice catch. That has unsurprisingly revealed some wrong assumptions I had about the scheduling. I had it in my head that for a 3 element stack it would only need 1 action to solve, if unordered was allowed. And so I made the test and it passed. Wow. But why was the total actions so high? Dunno, whatever, it passes.
But of course this makes it pass. The assumption I had was true, but only for Move/Move, so I've updated the test. Thanks.
|
|
||
| #[test] | ||
| fn every_ordered_stack() { | ||
| // Take every permutation of a 5 element stack and each permutation of two operand contraints |
There was a problem hiding this comment.
| // Take every permutation of a 5 element stack and each permutation of two operand contraints | |
| // Take every permutation of a 5 element stack and each permutation of two operand constraints |
There was a problem hiding this comment.
I can't tell you how often I write 'contraint' instead of 'constraint'. Some sort of glitch.
| } | ||
|
|
||
| fn every_duplicated_stack_single_util(may_be_unordered: bool) -> usize { | ||
| // Take very permutation of a 4 element stack etc. where the two operands are the very same |
There was a problem hiding this comment.
| // Take very permutation of a 4 element stack etc. where the two operands are the very same | |
| // Take every permutation of a 4 element stack etc. where the two operands are the very same |
| if is_solved { | ||
| return Err(SolverError::AlreadySolved); | ||
| } | ||
|
|
||
| Ok(Self { | ||
| stack, | ||
| expected: expected_output, | ||
| unordered: may_be_unordered, |
There was a problem hiding this comment.
Consider naming both (field and parameter) the same name. As a side note, would allow_unordered fit better?
There was a problem hiding this comment.
Thanks. This really bugged me. I had can_be_unordered and thought yuck, but then couldn't think of anything better and used may_be_unordered in the end, but still yuck. allow_unordered is much better. 🤦 I've changed it everywhere. The predicate to query it is .unordered_allowed() though since it's a little better there.
| @@ -147,6 +157,7 @@ impl OperandMovementConstraintSolver { | |||
| self.tactics.push(Box::new(Linear)); | |||
| self.tactics.push(Box::new(CopyAll)); | |||
| } | |||
| self.tactics.push(Box::new(TwoArgs)); | |||
There was a problem hiding this comment.
Consider adding a debug_assert before running the solver to ensure TwoArgs is running first. Some red light if we mess with the order in the future.
There was a problem hiding this comment.
I looked at this but whatever I could think of to put into an assert either couldn't work or was intrusively gnarly.
I think if the TwoArgs isn't last (or popped first) then the integration tests will probably fail, since none of the other tactics will account for commutivity. But your point is if/when that happens it might not be obvious why, where an assert would clarify that.
There was a problem hiding this comment.
If the test only fails because of the expected file changes, it's easy to miss. I'd even put the assert right after this line so it'd also serve as documentation of why it should be first (to pop).
This PR has two commits and should probably be reviewed per commit.
The first commit changes the solver to use stacks of
ValueOrAliasrather thanOperand. The old code tries very hard to keep the.posmember consistent with the place it sits in the stack, and by removing it we can instead just use the literal position of the value in the stack.The second commit takes the binary op special case handling out of the top of
lowering.rsand puts it into a proper tactic for the solver to use. It still creates a solution for every possible binary operation though, and will take priority for binary ops.To handle commutative ops, which is where most of the benefits for special casing come from, a
may_be_unorderedflag is sent deep into the solver. When checking if a solution is valid it now checks for exact matching, or a relaxed match if the output may be unordered.Testing for the new tactic uses full permutations of every possible input and ensures the output is valid, has at most 2 actions and has a check to ensure no regressions.
One part I'm unsure of, and is commented out in this PR, is handling cases where there are multiple copies of a single value already in the input stack and especially the case where they should both be moved.
When building the context for the solver any dupes are found and converted into copies, regardless of the actual passed constraints. I'm not sure why this is a thing, but when I attempted to test this scenario it foiled my efforts. :) @bitwalker maybe you know why it's done and if it's still necessary. Is it perhaps impossible for the input to have multiple copies of the same value? I would guess not.
Closes #614.