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

WIP: add support for working with immutables (#11902) #12113

Closed
wants to merge 3 commits into from
Closed

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 11, 2015

This is WIP towards #11902. Only bits tuples are supported, and this
has only so far been tested on Mac/LLVM37

Keno added 3 commits July 10, 2015 23:02
This is WIP towards #11902. Only bits tuples are supported, and this
has only so far been tested on Mac/LLVM37
@Keno
Copy link
Member Author

Keno commented Jul 11, 2015

This basically works now, but it's not particularly useful, because the intermediate ref allocation does still happen, so even something simple like

function -{N,T}(x::NTuple{N,T})
y = Ref{NTuple{N,T}}()
for i = 1:N
y[i] = -x[i]
end
y.x
end

ends up allocating. @JeffBezanson do we have anything in there right now that could deal with this?

@vtjnash
Copy link
Member

vtjnash commented Jul 13, 2015

i was expecting this to be implemented via the existing pointerref / pointerset intrinsics (possible with the addition of a new gep intrinsic). what's the rationale for adding this as a builtin function instead?

@Keno
Copy link
Member Author

Keno commented Jul 13, 2015

I was concerned about GC behavior, which is why I added a new intrinsic that atomically does this so no random pointers are flying around.

@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2015

I was concerned about GC behavior, which is why I added a new intrinsic that atomically does this so no random pointers are flying around.

fair enough. a lot of the Ref code is built around late / lazy conversion to pointer under the premise that we can use that to ensure Ref ends up assigned to some local variable before having unsafe_convert called on it.

since setindex! returns the Ref, I think that ensures GC must keep it alive.

@Keno
Copy link
Member Author

Keno commented Jul 14, 2015

Except it doesn't. It returns the assigned value.

@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2015

Except it doesn't. It returns the assigned value.

it isn't supposed to:

julia> x=[1,2,3]
3-element Array{Int64,1}:
 1
 2
 3

julia> setindex!(x, 5, 2)
3-element Array{Int64,1}:
 1
 5
 3

@Keno
Copy link
Member Author

Keno commented Jul 14, 2015

Oh, huh, why did I think it returned the assigned value?

@Keno
Copy link
Member Author

Keno commented Jul 14, 2015

Oh, yeah:

julia> a = [1,2,3]
3-element Array{Int64,1}:
 1
 2
 3

julia> a[1] = 2
2

@Keno
Copy link
Member Author

Keno commented Jul 14, 2015

In any case though, after inlining is the Ref still guaranteed to be live?

@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2015

it doesn't really matter if it's alive or not once you've lost the reference to it, since you wouldn't be able to take the final value back out anyways

@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2015

what if the base case (and new intrinsic) was offsetof, rather than mimicking GEP or the C behavior for this function.

then the implementation code could look something like:

function setindex!{T}(ref::Ref{T}, val, indices)
  ptr = unsafe_convert(::Ptr{T}, ref)
  offset = Intrinsics.offsetof(T, indices)
  unsafe_store!(ptr + offset, val)
  ref
end

@IainNZ IainNZ added this to the 0.4.0 milestone Jul 21, 2015
@IainNZ
Copy link
Member

IainNZ commented Jul 21, 2015

(tagged 0.4.0 because underlying issue is also tagged 0.4.0)

@Keno Keno modified the milestones: 0.5, 0.4.0 Jul 27, 2015
@mbauman mbauman mentioned this pull request Sep 15, 2015
27 tasks
@JeffBezanson JeffBezanson modified the milestones: 0.6.0, 0.5.0 Apr 21, 2016
@StefanKarpinski
Copy link
Member

Not enough progress has been made, kicking to 1.0.

@StefanKarpinski StefanKarpinski modified the milestones: 1.0, 0.6.0 Dec 15, 2016
@StefanKarpinski StefanKarpinski changed the title WIP: Implement #11902 WIP: Implement more support for working with immutables (#11902) Jul 20, 2017
@StefanKarpinski StefanKarpinski changed the title WIP: Implement more support for working with immutables (#11902) WIP: add more support for working with immutables (#11902) Jul 20, 2017
@StefanKarpinski StefanKarpinski changed the title WIP: add more support for working with immutables (#11902) WIP: add support for working with immutables (#11902) Jul 20, 2017
@StefanKarpinski StefanKarpinski modified the milestones: 1.x, 1.0 Jul 27, 2017
@Keno
Copy link
Member Author

Keno commented Jul 27, 2017

Superseded by #21912

@DilumAluthge DilumAluthge deleted the kf/11902 branch March 25, 2021 22:12
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.

5 participants