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

Fix large dynamic tape issue #509

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Fix large dynamic tape issue #509

merged 3 commits into from
Oct 12, 2022

Conversation

wsmoses
Copy link
Member

@wsmoses wsmoses commented Oct 12, 2022

Fixes #500

@wsmoses wsmoses requested a review from vchuravy October 12, 2022 00:04
@vchuravy
Copy link
Member

That is illegal. NTuple is an immutable so despite it being allocated on the heap we shouldn't mutate it after it has been observed

@wsmoses
Copy link
Member Author

wsmoses commented Oct 12, 2022

We're not mutating it after being observed, we're initializing it?

@vchuravy
Copy link
Member

We are guaranteed to never update values in place? That was my assumption for why it had to be a mutable

@wsmoses
Copy link
Member Author

wsmoses commented Oct 12, 2022

thinking this through:

This technically overrides the Enzyme internal allocator in its entirety.

We would only have a julia object internally if we needed to cache it for a tape. The tape should be immutable once initialized (technically we may memset it to 0 -- in fact in this very julia_allocator function), but otherwise once its value is set to actually preserve something, it is the same. We even attach invariant.group MD on it

src/compiler.jl Outdated Show resolved Hide resolved
Co-authored-by: Valentin Churavy <[email protected]>
test/runtests.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2022

Codecov Report

Base: 74.87% // Head: 74.61% // Decreases project coverage by -0.26% ⚠️

Coverage data is based on head (2f170b6) compared to base (5bfb57a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
- Coverage   74.87%   74.61%   -0.27%     
==========================================
  Files          17       17              
  Lines        5370     5370              
==========================================
- Hits         4021     4007      -14     
- Misses       1349     1363      +14     
Impacted Files Coverage Δ
src/compiler.jl 73.75% <100.00%> (-0.03%) ⬇️
src/compiler/orcv2.jl 61.90% <0.00%> (-10.48%) ⬇️
src/compiler/validation.jl 62.06% <0.00%> (-0.63%) ⬇️

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

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

@wsmoses wsmoses merged commit e452f89 into main Oct 12, 2022
@wsmoses wsmoses deleted the ldyntape branch October 12, 2022 02:00
@wsmoses wsmoses mentioned this pull request Feb 14, 2024
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.

Segmentation fault on a linear regression model (arm & x86 chips)
3 participants