-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Adding declarations to llvmcall #11604
Conversation
llvmcall("""%2 = call double @llvm.ceil.f64(double %0) | ||
ret double %2""", Float64, Tuple{Float64}, x) | ||
end | ||
@test_throws ErrorException undeclared_ceil(4.2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is apparently failing - does this functionality only work with LLVM 3.5+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test seems to be quite finicky
Executing from the REPL:
julia> function undeclared_ceil(x::Float64)
Base.llvmcall("""%2 = call double @llvm.ceil.f64(double %0)
ret double %2""", Float64, Tuple{Float64}, x)
end
undeclared_ceil (generic function with 1 method)
julia> undeclared_ceil(4.2)
ERROR: error compiling undeclared_ceil: Failed to parse LLVM Assembly:
julia: <string>:3:18: error: use of undefined value '@llvm.ceil.f64'
%2 = call double @llvm.ceil.f64(double %0)
try
undeclared_ceil(4.3)
catch err
println(err)
end
ErrorException("error compiling undeclared_ceil: Failed to parse LLVM Assembly: \njulia: <string>:3:18: error: use of undefined value '@llvm.ceil.f64'\n%2 = call double @llvm.ceil.f64(double %0)\n ^\n")
julia> Test.@test_throws ErrorException undeclared_ceil(4.3)
julia> Test.@test_throws ErrorException undeclared_ceil(4.3)
ERROR: test failed: undeclared_ceil(4.3) did not throw ErrorException
in expression: undeclared_ceil(4.3)
in error at error.jl:21
in default_handler at test.jl:29
in do_test_throws at test.jl:74
julia> try
undeclared_ceil(4.3)
catch err
println(typeof(err))
end
ErrorException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I noticed similar unpredictable behavior. Maybe the GC root will help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I really doubt... But maybe.....
The GC root needs to be fix though, unless I'm missing sth.
Edit: and by "the GC root" I meant the new add gc root commit
// if the IR is a tuple, we expect (declarations, ir) | ||
if (jl_nfields(ir) != 2) | ||
jl_error("Tuple as first argument to llvmcall must have exactly two children"); | ||
decl = jl_fieldref(ir,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing GC root? (ir
is overriden below and (new) line 628
is allocating)
There is one problem that I see and I don't for certain how to best solve it. Looking at the test failure wrt
Even better calling a function with an undefined intrinsic throws the error and then adding a second function that also calls the intrinsic does not throws an error for the second function"
|
@maleadt added a comment about that case:
Suggestions are welcome |
jl_error("Tuple as first argument to llvmcall must have exactly two children"); | ||
decl = jl_fieldref(ir,0); | ||
ir = jl_fieldref(ir,1); | ||
// Need GC root because of allocating call later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces instead of tabs
I don't know whether there's any downside to enabling this feature, but if it would be useful to have now despite the tests doing funky things on LLVM 3.3, could we just comment out the unpredictable tests for now, and revisit after upgrading LLVM? #9336 |
I moved the gc root to the right place (hopefully) and commented out the wonky test. |
@vchuravy Yup the GC root looks good to me now =). |
Most of the added tests check the behaviour of llvm with regards to undefined or double defined intrinsics declarations, so for the time being I commented them out. |
Instead of out-commenting I decided to future proof the test by adding a conditional on them. So when #9336 occurs we don't forget about the tests. |
The Travis error on 64bit looks like a timeout to me, the llvmcall tests passed (Hurray). All test pass locally. |
So I think this is ready for another look from somebody else. The last commit extends llvmcall to take multiple lines of declarations |
Compilation works, but there are test errors:
|
@SimonDanisch what's your |
Ah yeah, that was not very informative.
Do I need LLVM 3.5, or what? |
and I did |
Thanks, I'll see if I can reproduce. Might be Haswell-specific? |
@SimonDanisch could you check out dff768a and see if the error is persistent there? That is the commit from which I would like to add some documentation for |
ref #10760 |
Looks quite similar:
|
@SimonDanisch thanks for checking that :) That means it is not the fault of this PR. Could you check against current master and if it persists open an issue about it? If it goes away fixing it here should be then as simple as a rebase. |
I'm on it ;) compilation is just always a little slow... |
Okay... So it first gave me an OutOfMemoryException(), but after a restart with fewer processes master completed successfully.
|
The test that checks llvm behaviour for undefined intrinsics does not work on llvm 3.3. It works(tm) under llvm 3.5+.
getNamedValue expects getNamedValue("llvm.floor.f64") for "declare double @llvm.floor.f64(double)"
@ViralBShah @hayd Rebased. Since we now have documentation for llvmcall I will write additional documentation for this. |
The failing build seems to be a homebrew issue.
|
Looked like a download failure, restarted the build and it passed. This LGTM if other people think it's safe. |
[ci skip]
So I added a short documentation for the declarations, I hope I got the format right. |
@Keno what do you think? |
Alright, tired of waiting for review from unresponsive code owners on this. If it causes any problems they can be addressed going forward. |
Adding declarations to llvmcall
Thank you all for staying on this with me :) |
Yeah that is next, but it will need a heavy rebase due to the codegen On Sun, 15 Nov 2015 at 23:27 Tony Kelman notifications@github.com wrote:
|
Thanks for getting this merged! I only now noticed the declaration extraction, why exactly do we do that? It also doesn't seem compatible with global variable declarations (a la |
I added the function name parsing to prevent adding already know declarations to the module, which would lead to an error (on llvm versions where Julia uses one module for everything). Since llvmcall is only meant for functions, I am unsure if adding more global state, is a good idea. What do you need global variables for? And would that work if each function has its own module? |
Previously, we just added the entire declaration string into the Concerning global variables, these just behave like declarations (only use them after being added once to the module). I'm currently using it for hackish shared memory support ( |
Hm the problem was that we would insert these declarations into a module where they already are defined. So as an example code like Lines 96 to 117 in 8c23dc8
|
But the declaration string for |
Yes if the declaration came in via llvmcall it will be caught, but not if it came in via one of the base intrinsics (I might be miss remembering things) |
Problem is, the intrinsics defined outside of |
Yes, that's why we need to parse them so we can check if they are already present in the module https://github.com/JuliaGPU/julia/commit/2ca11b6f5d39b4bf7d36aeb157871a2d663039df#diff-be60d844ffde1319a02946d17c472e46R629 |
Ah completely missed that, sorry for the noise. I'll update the declaration parsing then. |
Rebased from #8740 via #11516 in response to #11603
This allows to hook up llvm intrinsics via llvmcall. This is necessary for the CUDA backend work and several people expressed that they have other usages in mind.
Ping @ihnorton @tkelman
Overview
This extension of llvmcall allows to pass declarations of intrinsics to llvmcall. As an example see: