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

Implement fold_into_reduce and reassociate_expr #558

Merged
merged 5 commits into from
Jan 24, 2024
Merged

Conversation

SamirDroubi
Copy link
Collaborator

This PR implements two operations:

  1. fold_into_reduce: folds an assignment into a reduction format
  2. reassociate_expr: changes the associativity of addition and multiplication. This can be used to help with (1).

The primary reason behind this PR is being able to put assignments into reduction format which some analysis depends on (e.g. #557).

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fee83f5) 87.24% compared to head (d764e6e) 87.26%.

Files Patch % Lines
tests/test_schedules.py 94.73% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #558      +/-   ##
==========================================
+ Coverage   87.24%   87.26%   +0.02%     
==========================================
  Files          82       82              
  Lines       19416    19487      +71     
==========================================
+ Hits        16939    17006      +67     
- Misses       2477     2481       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SamirDroubi SamirDroubi requested review from yamaguchi1024 and removed request for skeqiqevian January 24, 2024 04:37
@SamirDroubi
Copy link
Collaborator Author

I updated the name to left_reassociate_expr and changed the rewrite of fold_into_reduce to work on the lhs of the rhs of the assignment. I just still don't have a good name for it.

Some ideas:

  • squash_into_reduce
  • compound_assignment

@yamaguchi1024
Copy link
Member

left_reassociate_expr looks good because it doesn't care about the content of A, B, and C as long as they are in this AST form:
PXL_20240124_203500468

I still think it's confusing for fold_into_reduce to accept the left figure while not accepting the right figure, even though they both look like a = a + b + ... in the frontend syntax.
PXL_20240124_203720814

@SamirDroubi
Copy link
Collaborator Author

They look the same because we simplify the parenthesis. My experience with languages that implements expression rewrites (like Coq) is that you just have to pay attention to these things when using the primitives. Then, you build hammer automations on top of it.

@yamaguchi1024
Copy link
Member

I agree that we should build automation in the standard library, but at least the primitive definition should be consistent with what it is actually doing. The current documentation writes

    rewrite:
        a = a + ...
            ->
        a += ...

which doesn't accurately describe the acceptable program. You could change the documentation to describe the left figure above, but I think it will be hard to write the documentation with syntax that is opaque to users (binop AST).

@SamirDroubi
Copy link
Collaborator Author

We can change it to:

    rewrite:
        a = a + (...)
            ->
        a += ...

We also need to a better name that signifies what it does.

@yamaguchi1024
Copy link
Member

Yeah I guess a = a + (...) is consistent. Also, folding reduction is necessary for circumventing the reorder_loop bug, right? So hopefully this won't be an issue when we have better analysis

@SamirDroubi SamirDroubi enabled auto-merge (squash) January 24, 2024 21:58
@SamirDroubi SamirDroubi merged commit d1d4db1 into main Jan 24, 2024
6 checks passed
@SamirDroubi SamirDroubi deleted the fold-into-reduce branch January 24, 2024 22:23
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.

None yet

3 participants