add chance of reverse chain assignment in perm_temp_for_expr #138
Conversation
|
can you give an example of what this aims to do? |
|
Yep, in perm_temp_for_expr, sometimes it will introduce a new_var and assign it in the first expression it's used. Like so: This adds a 50/50 chance to do the assignment like this: which fixes some regswaps |
snuffysasa
force-pushed
the
reverse_chain_assign_w_new_var
branch
from
dd23629
to
eadec51
Compare
|
rebased to latest main branch and resolved merge conflicts ready for more review, but not a big priority. |
Sorry, I'm failing to line this description up with the code. It seems to do something else, involving changing which expression gets replaced. Or maybe I'm just misunderstanding the code, in which I need comments on it explaining what each thing does... (this may be good in either case) Until then I can't give a proper review. |
| ast_util.for_nested_blocks(stmt, rec) | ||
| return None | ||
|
|
||
| rec(block) |
There was a problem hiding this comment.
simpler to use a visitor for this rather than explicit recursion
There was a problem hiding this comment.
hmm I think it was not possible to use a visitor here. I'll explain what the issue was when I get back to this
| if place is None and not should_make_ptr: | ||
| stmt = find_assignment_stmt_by_rvalue(fn.body, expr) | ||
| if stmt and random_bool(random, 0.5): | ||
| assert isinstance(stmt, ca.Assignment) |
There was a problem hiding this comment.
this assert shouldn't be needed
| @@ -712,18 +744,19 @@ def find_duplicates(e: Expression) -> None: | |||
| else: | |||
| replace_subexprs(fn.body, find_duplicates) | |||
|
|
|||
| assert orig_expr in replace_cands | |||
| replace_cand_set: Set[Expression] = set() | |||
| if random_bool(random, PROB_TEMP_REPLACE_ALL): | |||
There was a problem hiding this comment.
(I changed this part of the code in a recent commit, will need to be updated)
Yea makes sense, it was quite tricky to implement this. I will try to make the code more clear with comments in a bit. |
|
@snuffysasa did you determine this to be useful / has it been helping you? |
This was complicated to implement and took me awhile, and probably only limited benefit.
If its too much work to get it merged in, or don't like it, I may just end up using it on my fork for a while and see how it goes