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.

Sign up for GitHub

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

Add more types to the returned allocation #19

Merged
merged 9 commits into from
Nov 6, 2023
Merged

Add more types to the returned allocation #19

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7da2acf
Add more types to the returned allocation
gbaraldi Oct 20, 2023
2f79357
Add throw(EOFError()) check
gbaraldi Oct 30, 2023
41c3931
Fix test result that changes
gbaraldi Oct 30, 2023
a07a4a2
Address comments from review
gbaraldi Oct 30, 2023
f71421b
Fix typo and add assert
gbaraldi Oct 31, 2023
47188ae
Change logic slightly so we do an extensive check
gbaraldi Oct 31, 2023
ddd6af4
Merge branch 'main' into gb/more_types
gbaraldi Oct 31, 2023
cf363bf
Remove unnecessary return statement
gbaraldi Nov 6, 2023
f8eae41
Update test/runtests.jl
gbaraldi Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 56 additions & 2 deletions src/allocfunc.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =#
Copy link
Member

Choose a reason for hiding this comment

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

A good test for this is to check that foo() = throw(EOFError()) is considered non-allocating

"jl_gc_queue_root", "ijl_gc_queue_root",
)

const known_alloc_with_throw_funcs = (
Expand Down Expand Up @@ -73,9 +75,8 @@ function guess_julia_type(val::LLVM.Value, typeof=true)
end
if isa(val, LLVM.CallInst) && typeof
fn = LLVM.called_operand(val)
if isa(fn, LLVM.Function) && (LLVM.name(fn) in ("ijl_gc_pool_alloc_instrumented", "ijl_gc_big_alloc_instrumented", "ijl_gc_alloc_typed"))
if isa(fn, LLVM.Function) && (LLVM.name(fn) in ("ijl_gc_pool_alloc_instrumented", "ijl_gc_big_alloc_instrumented", "ijl_gc_alloc_typed", "jl_gc_pool_alloc_instrumented", "jl_gc_big_alloc_instrumented", "jl_gc_alloc_typed"))
res = guess_julia_type(operands(val)[end-1], false)

if res !== nothing
return res
end
Expand All @@ -87,10 +88,63 @@ function guess_julia_type(val::LLVM.Value, typeof=true)
return res
end
end

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
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way for us to know the type of the array?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, grepping for this function in the runtime brings up nothing for me. Do you know what's up?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is typo of mine, it should be jl_array_copy

Copy link
Member

Choose a reason for hiding this comment

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

I think this still needs fixing?

end
if isa(fn, LLVM.Function) && occursin(r"(ijl_|jl_)box_(.*)", name(fn))
typestr = match(r"(ijl_|jl_)box_(.*)", name(fn)).captures[end]
typestr == "bool" && return Bool
typestr == "char" && return Char
typestr == "float32" && return Float32
typestr == "float64" && return Float64
typestr == "int16" && return Int16
typestr == "int32" && return Int32
typestr == "int64" && return Int64
typestr == "int8" && return Int8
typestr == "slotnumber" && return Core.SlotNumber
typestr == "ssavalue" && return Core.SSAValue
typestr == "uint16" && return UInt16
typestr == "uint32" && return UInt32
typestr == "uint64" && return UInt64
typestr == "uint8" && return UInt8
typestr == "uint8pointer" && return Ptr{UInt8}
typestr == "voidpointer" && return Ptr{Cvoid}
return Any
end

if isa(fn, LLVM.Function) && in(LLVM.name(fn), ("ijl_gc_pool_alloc", "jl_gc_pool_alloc"))
for use in uses(val)
istag = user(use)
if isa(istag, LLVM.BitCastInst)
for bituse in uses(istag)
isgep = user(bituse)
if isa(isgep, LLVM.GetElementPtrInst)
Copy link
Member

Choose a reason for hiding this comment

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

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))

istag = isgep
@goto gep_handling
end
end
elseif isa(istag, LLVM.GetElementPtrInst)
@goto gep_handling
else
continue
end
@label gep_handling
if convert(Int,operands(istag::LLVM.GetElementPtrInst)[2]) == -1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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).

Copy link
Member

Choose a reason for hiding this comment

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

Also I think this needs to check isa(offset, LLVM.ConstantInt) otherwise the convert will fail for anything with a runtime offset

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we are lacking the accessor functions :(. I need to add some getSrcType/getDestType calls to LLVM.jl

Copy link
Member Author

Choose a reason for hiding this comment

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

But I don't think we do a -1 gep on anything except the tag.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it looks like LLVM.LLVMType(LLVM.API.LLVMGetGEPSourceElementType(gepinst))) is implemented

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, great we do wrap it. We don't have most of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's only in LLVM 15 :(

for gepuse in uses(istag)
isstore = user(gepuse)
if isa(isstore, LLVM.StoreInst)
return guess_julia_type(operands(isstore)[1], true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Member Author

Choose a reason for hiding this comment

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

guess_julia_type dispatches on the type, so this is a bit unnecessary

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it costs us nothing to do it.

end
end
end
end
return guess_julia_type(operands(val)[1], false)
end
break
end
break
Expand Down
7 changes: 6 additions & 1 deletion test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ function same_ccall()
a,b
end

function throw_eof()
throw(EOFError())
end

@testset "AllocCheck.jl" begin
@test length(check_allocs(mod, (Float64,Float64))) == 0
@test length(check_allocs(sin, (Float64,); ignore_throw=false)) > 0
Expand All @@ -28,7 +32,8 @@ end
@test length(check_allocs(same_ccall, (), ignore_throw=false)) == 2
@test length(check_allocs(same_ccall, (), ignore_throw=true)) == 2

@test length(check_allocs(first, (Core.SimpleVector,); ignore_throw = false)) == 3
@test length(check_allocs(first, (Core.SimpleVector,); ignore_throw = false)) == 2
@test length(check_allocs(first, (Core.SimpleVector,); ignore_throw = true)) == 0
@test length(check_allocs(time, ())) == 0
@test length(check_allocs(throw_eof, ())) == 0
end
Loading