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

Improve type inference of non-const GlobalRef's #168

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Conversation

staticfloat
Copy link
Collaborator

GlobalRefs often do not get their types refined, so we should be careful to re-use types that we have when possible. This PR moves non-const GlobalRef handling out of maparg() so that we can keep the original type.

@staticfloat staticfloat requested review from Keno and oxinabox June 27, 2023 22:26
@oscardssmith
Copy link
Member

Needs a test, but otherwise looks good.

@staticfloat
Copy link
Collaborator Author

Test added, luckily it was not as difficult as I feared. I have verified that the test fails without this change. Unfortunately, the reverse mode tests were broken by JuliaLang/julia@ec33194 and that might be a heavy lift to fix. We'll check in with Keno tomorrow to see how bad it's going to be to fix that (he says it's a miscompile, which sounds bad). If it's too too painful we can either disable those reverse-mode tests (since we're not using them anyway), look into some kind of workaround, or even merge this with the reverse mode tests failing.

GlobalRefs often do not get their types refined, so we should be careful
to re-use types that we have when possible.  This PR moves non-`const`
GlobalRef handling out of `maparg()` so that we can keep the original
type.
@oxinabox oxinabox force-pushed the sf/globalref_types branch from 72dc8e7 to e0963a9 Compare June 28, 2023 07:39
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch coverage: 42.85% and no project coverage change.

Comparison is base (53a3ded) 22.96% compared to head (e0963a9) 22.96%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #168   +/-   ##
=======================================
  Coverage   22.96%   22.96%           
=======================================
  Files          27       27           
  Lines        2661     2665    +4     
=======================================
+ Hits          611      612    +1     
- Misses       2050     2053    +3     
Impacted Files Coverage Δ
src/codegen/forward_demand.jl 44.59% <42.85%> (-0.36%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@oxinabox oxinabox merged commit da2c0bb into main Jun 28, 2023
@oscardssmith oscardssmith deleted the sf/globalref_types branch June 28, 2023 11:42
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.

3 participants