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

Change __getindex_dims to work better with inference #100

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

ianatol
Copy link
Contributor

@ianatol ianatol commented Aug 30, 2022

While working on the upcoming Core.Compiler change JuliaLang/julia#44803, we discovered that inference had some problems seeing through _getindex_dims to do proper constant propagation. Currently this isn't a problem, but in the semi-concrete branch it caused a loss of accuracy. This change fixes that pre-emptively, and shouldn't have any effect otherwise.

CC: @Keno

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ianatol.

Any idea how much faster your changes will make compiling typed tables with many columns?

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #100 (bc1745a) into main (145b0fa) will decrease coverage by 0.22%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   69.63%   69.41%   -0.23%     
==========================================
  Files           7        7              
  Lines         909      909              
==========================================
- Hits          633      631       -2     
- Misses        276      278       +2     
Impacted Files Coverage Δ
src/FlexTable.jl 80.60% <80.00%> (ø)
src/properties.jl 78.94% <0.00%> (-0.76%) ⬇️
src/Table.jl 73.41% <0.00%> (-0.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andyferris andyferris merged commit 03cc7d2 into JuliaData:main Aug 31, 2022
@ianatol
Copy link
Contributor Author

ianatol commented Aug 31, 2022

@andyferris I'm not sure if there will be a hugely noticeable performance benefit here, though there may be places where performance is more predictable. The problem that this compiler work aims to solve is improving performance of functions that are just ineligible for concrete evaluation, and generally smoothing the performance valley between concrete evaluation and normal constant propagation. If there are many such functions internal to typed table operations then it is possible that there may be a speed increase.

@ianatol ianatol deleted the ia/constprop branch August 31, 2022 04:03
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.

2 participants