Add more types to the returned allocation #19
Conversation
| @@ -19,6 +19,8 @@ const known_nonalloc_funcs = ( | |||
| "jl_pop_handler", "ijl_pop_handler", | |||
| "jl_f_typeof", "ijl_f_typeof", | |||
| "jl_clock_now", "ijl_clock_now", | |||
| "jl_throw", "ijl_throw", #= the allocation of the error is separate =# | |||
There was a problem hiding this comment.
A good test for this is to check that foo() = throw(EOFError()) is considered non-allocating
gbaraldi
force-pushed
the
gb/more_types
branch
from
14188e4 to
41c3931
Compare
| if isa(fn, LLVM.Function) && in(LLVM.name(fn), ("ijl_alloc_string", "jl_alloc_string")) | ||
| return String | ||
| end | ||
|
|
||
| if isa(fn, LLVM.Function) && in(LLVM.name(fn), ("ijl_copy_array", "jl_copy_array")) | ||
| return Array |
There was a problem hiding this comment.
Is there any way for us to know the type of the array?
There was a problem hiding this comment.
Hmm, grepping for this function in the runtime brings up nothing for me. Do you know what's up?
There was a problem hiding this comment.
That is typo of mine, it should be jl_array_copy
There was a problem hiding this comment.
I think this still needs fixing?
src/allocfunc.jl
Outdated
| continue | ||
| end | ||
| @label gep_handling | ||
| if convert(Int,operands(istag::LLVM.GetElementPtrInst)[2]) == -1 |
There was a problem hiding this comment.
| if convert(Int,operands(istag::LLVM.GetElementPtrInst)[2]) == -1 | |
| offset = operands(istag::LLVM.GetElementPtrInst)[2] | |
| if convert(Int, offset) == -1 |
Also, ideally this would also inspect the type of the GEP so that we know exactly what byte offset this is in memory (and the size of the store).
There was a problem hiding this comment.
Also I think this needs to check isa(offset, LLVM.ConstantInt) otherwise the convert will fail for anything with a runtime offset
There was a problem hiding this comment.
I think we are lacking the accessor functions :(. I need to add some getSrcType/getDestType calls to LLVM.jl
There was a problem hiding this comment.
But I don't think we do a -1 gep on anything except the tag.
There was a problem hiding this comment.
Hmm, it looks like LLVM.LLVMType(LLVM.API.LLVMGetGEPSourceElementType(gepinst))) is implemented
There was a problem hiding this comment.
Oh, great we do wrap it. We don't have most of these.
There was a problem hiding this comment.
it's only in LLVM 15 :(
src/allocfunc.jl
Outdated
| for gepuse in uses(istag) | ||
| isstore = user(gepuse) | ||
| if isa(isstore, LLVM.StoreInst) | ||
| return guess_julia_type(operands(isstore)[1], true) |
There was a problem hiding this comment.
| return guess_julia_type(operands(isstore)[1], true) | |
| type_tag = operands(isstore)[1] | |
| @assert type_tag isa LLVM.ConstantInt | |
| return guess_julia_type(type_tag, true) |
There was a problem hiding this comment.
guess_julia_type dispatches on the type, so this is a bit unnecessary
There was a problem hiding this comment.
All asserts are unnecessary - this is to make explicit our assumptions on the IR.
Do we expect this to not be constant sometimes? If so, can guess_julia_type still give us a reasonable result?
There was a problem hiding this comment.
I guess it costs us nothing to do it.
| if isa(istag, LLVM.BitCastInst) | ||
| for bituse in uses(istag) | ||
| isgep = user(bituse) | ||
| if isa(isgep, LLVM.GetElementPtrInst) |
There was a problem hiding this comment.
I don't think this logic is correct, since it will, e.g., ignore a bitcast whose second usage is the -1 GEP offset but whose first usage is a different GEP
It might be simpler to introduce something like transitive_uses(val; unwrap = (x)->isa(x, LLVM.BitcastInst))
There was a problem hiding this comment.
Looking good to me! Thanks for the iteration here @gbaraldi
Only last thought is that it'd be good to assert that we were able to find a type successfully, since it sounds like we expect these calls to always be lowered to LLVM with compile-time constant type-tag parameters.
Co-authored-by: Cody Tapscott <[email protected]>
This needs tests, but should bring the tpyes of allocations in 1.10/9 should be more correct.