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

compiler: Improve IndexDerivative lowering #2288

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Conversation

FabioLuporini
Copy link
Contributor

Tiny enhancement to avoid useless stupid temporary variables in the generated code

@FabioLuporini
Copy link
Contributor Author

"Fabio, have you thought about just performing a round of cse() instead"

"Yes I have, unfortunately it's not that easy because CSE is a Cluster-local pass, which doesn't work well here"

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c888cee) 86.76% compared to head (df4e53d) 86.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2288   +/-   ##
=======================================
  Coverage   86.76%   86.76%           
=======================================
  Files         229      229           
  Lines       42884    42893    +9     
  Branches     7951     7953    +2     
=======================================
+ Hits        37207    37216    +9     
  Misses       5002     5002           
  Partials      675      675           

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

@georgebisbas
Copy link
Contributor

georgebisbas commented Dec 22, 2023

"Fabio, have you thought about just performing a round of cse() instead"

"Yes I have, unfortunately it's not that easy because CSE is a Cluster-local pass, which doesn't work well here"

Why does it not work well? Not sure I understand TBH

@FabioLuporini
Copy link
Contributor Author

Why does it not work well? Not sure I understand TBH

lower_index_derivative might produce:

Cluster([
  r1 = 0
  r1 += <lowered index derivative>
  r0 = r1
])

CSE would be able to see that r0 is unnecessary here and safely removable. But r0 is needed in later Clusters, so it would produce wrong code ultimately

Note that this is just a simplified example

You would need to understand lower_index_derivatives fully to appreciate all the different flavors of this issue

@georgebisbas
Copy link
Contributor

Why does it not work well? Not sure I understand TBH

lower_index_derivative might produce:

Cluster([
  r1 = 0
  r1 += <lowered index derivative>
  r0 = r1
])

CSE would be able to see that r0 is unnecessary here and safely removable. But r0 is needed in later Clusters, so it would produce wrong code ultimately

Note that this is just a simplified example

You would need to understand lower_index_derivatives fully to appreciate all the different flavors of this issue

Thanks

@FabioLuporini FabioLuporini merged commit f1db9d4 into master Jan 2, 2024
31 checks passed
@FabioLuporini FabioLuporini deleted the tweak-unexpansion branch January 2, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants