eliminate intrinsics by vtjnash · Pull Request #18754 · JuliaLang/julia

vtjnash · 2016-10-01T04:23:24Z

After this, only 1 intrinsics (llvmcall ~~and some ccall fixes~~) remain to go to be fully interpretable and statically compilable!

removes special parsing rules for ccall (and thus removes Expr(:ccall)), fixes Inconsistent parsing of ccall #18687
makes Expr(:call, :ccall) special in lowering (effectively, reserving ccall – as the keyword it already is) and which expands to Expr(:foreigncall) representing the lowered ffi call (instead of expanding to Expr(:call, Core.Intrinsics.ccall), even though this couldn't really be a first-class function)
adds error checking to all Core.Intrinsic functions
adds support to ccall for calling llvm intrinsics directly (via "llvmcall" calling convention)
the unbox intrinsic is now just a deprecated alias for the box intrinsic (aka reinterpret)

The ccall intrinsic now evaluates its global arguments during method definition time, eliminating the call back into the runtime from inside codegen (yay!), and making its semantics independent of runtime values (so that its behavior is statically compilable).

edit: see https://github.com/vtjnash/julep/wiki/0001:-Enhanced-static-compilation-and-C-interface

Sacha0 · 2016-10-01T17:07:06Z

base/floatfuncs.jl

-copysign(x::Float64, y::Float64) = box(Float64,copysign_float(unbox(Float64,x),unbox(Float64,y)))
-copysign(x::Float32, y::Float32) = box(Float32,copysign_float(unbox(Float32,x),unbox(Float32,y)))
+copysign(x::Float64,  y::Float64) = box(Float64, copysign_float(x, y))
+copysign(x::Float32,  y::Float32) = box(Float32, copysign_float(x, y))


Extra space preceding y in the method signature?

Sacha0 · 2016-10-01T17:07:14Z

base/floatfuncs.jl

-flipsign(x::Float64, y::Float64) = box(Float64,xor_int(unbox(Float64,x),and_int(unbox(Float64,y),0x8000000000000000)))
-flipsign(x::Float32, y::Float32) = box(Float32,xor_int(unbox(Float32,x),and_int(unbox(Float32,y),0x80000000)))
+flipsign(x::Float64,  y::Float64) = box(Float64, xor_int(box(UInt64, x), and_int(box(UInt64, y), 0x8000000000000000)))
+flipsign(x::Float32,  y::Float32) = box(Float32, xor_int(box(UInt32, x), and_int(box(UInt32, y), 0x80000000)))


Extra space preceding y in the method signature here as well?

maleadt · 2016-10-02T15:30:44Z

Cool, the llvmcall calling convention will make some of our llvmcall(...) usage in CUDAnative redundant. What about function attributes though? Most NVPTX intrinsics are readnone nounwind.

For PTX we also need to call into libdevice, an IR blob providing implementations of common math functions. This blob is linked in during JIT, so regular ccall doesn't work (disregarding the fact that there's no dlopen, etc). Bottom line, it would be handy if the new llvmcall cc wouldn't check for isIntrinsic, even though it makes sense to require that for every other sane use case...

tkelman · 2016-10-03T05:57:28Z

doc/manual/calling-c-and-fortran-code.rst

+
+.. _CUDA: http://llvm.org/docs/NVPTXUsage.html
+
+As with any ccall, it is essential to get the argument signature exactly correct.


code highlight ccall

tkelman · 2016-10-03T06:07:34Z

src/ccall.cpp

+        jl_error(msg.c_str()); // TODO: this leaks memory
+    }
+    if (jl_is_cpointer_type(rt) && jl_is_typevar(jl_tparam0(rt)))
+        jl_error("ccall: return type Ptr should have an element type, not Ptr{_<:T}");


this Ptr{_<:T} will be pretty confusing

this isn't a new change

tkelman · 2016-10-03T07:28:41Z

src/intrinsics.cpp

 {
+    if (t->isFloatingPointTy())
+        return t;
+    unsigned nb = (t->isPointerTy() ? t->getPrimitiveSizeInBits() : sizeof(void*));


is this backwards?

needs-test then if it was passing with a mistake here

tkelman · 2016-10-03T07:30:44Z

src/intrinsics.cpp

+    if (t == T_float16)
+        return T_int16;
+    //if (t == T_float128)
+    //    return T_int128;


leave a comment or a cross-reference about why this is commented out (same below in JL_JLUINTT and JL_JLSINTT)

this isn't a new change

that's not a reason to leave unexplained things or lousy error messages in the code. while it's being rearranged is the right time to make these little improvements

tkelman · 2016-10-03T07:47:27Z

src/intrinsics.cpp

-#else
-    Value *ret = builder.CreateCall3(prepare_call(jlpref_func), boxed(parg, ctx), iarg, alignarg);
+#ifdef JL_NEED_FLOATTEMP_VAR
+        // Target platform might carry extra precision.


excess indent

tkelman · 2016-10-03T08:33:01Z

needs a test for #18687

vtjnash · 2016-10-03T16:27:46Z

this isn't that kind of fix. this "fixes" the issue by deleting the special parsing code and the tests for it (of which apparently there were none).

tkelman · 2016-10-03T16:41:06Z

doesn't matter how it's being fixed. add a test.

vtjnash · 2016-10-03T16:47:52Z

It's not fixed, it's deleted, along with all tests for it.

tkelman · 2016-10-03T22:28:58Z

That issue is about the fact that it works differently on master. To close that issue, and just in general, should add tests that it no longer works differently in this pr. Not asking for much here, tests and comments. Why are you fighting that?

vtjnash · 2016-10-04T03:00:43Z

I'm not fixing that issue, I'm deleting the feature that the referenced issue complained about.

tkelman

So what? You're replacing it with a new implementation that needs to be tested that it no longer has the undesirable behavior.

vtjnash · 2016-10-04T03:49:53Z

So what? You're replacing it with a new implementation that needs to be tested that it no longer has the undesirable behavior.

No, it's completely gone. The parser no longer has any knowledge of ccall.

tkelman · 2016-10-04T03:59:30Z

The implementation now happening in lowering instead of parsing doesn't change the fact that the parser behavior change for ccall should be tested and verified.

vtjnash · 2016-10-04T04:23:44Z

How does the parser not implementing ccall not eliminate the need to test how the parser implements ccall? The parser no longer has any special behavior related to the symbol ccall, it is handled by exactly the same code as handles all Expr(:call) form expressions.

tkelman · 2016-10-04T04:36:11Z

Because the parser used to implement ccall. If it hadn't had a custom implementation before, it would be fine. This is a change in behavior specifically for ccall, and that's the part that should now be tested.

tkelman · 2016-10-05T03:47:32Z

why is llvmcall2 a separate file here? would the "nonsensical valid conversions and errors" tests have caught the mistake from #18754 (comment) ?

vtjnash · 2016-10-05T03:58:40Z

It tests v2 of llvmcall. Yes, it includes coverage of that case.

JeffBezanson · 2016-10-06T22:42:26Z

It occurs to me that after this change we should add t-functions for all intrinsics (previously we relied on their results always being passed to box).

vtjnash · 2016-10-07T02:51:12Z

Yes, that's also on my list of eventually fixes for here (along with completely removing unbox and removing most calls to box)

…anding to Expr(:ccall)

all intrinsics (but one) are now fully error-checking and runtime-interpreter-compatible

and remove the ducttape!

… T_pint8)

a handful of packages seem to have unused functions that are triggering this error

it was neutered by earlier commits, this just finishes deleting it

by adding type inference information to all of the intrinsics, the box intrinsic is no longer required for this purpose

this name better reflects what this intrinsic does, which is to change the type-tag on some data, without actually altering the data or its representation

Cannot use it for all intrinsics as the libdevice ones aren't true intrinsics, and are rejected by the new llvmcall. Ref JuliaLang/julia#18754

Ref JuliaLang/julia#18754

(cherry picked from commit e020772)

Sacha0 reviewed Oct 1, 2016

View reviewed changes

vtjnash force-pushed the jn/foreigncall branch from d912744 to 6574027 Compare October 2, 2016 22:26

tkelman reviewed Oct 3, 2016

View reviewed changes

vtjnash force-pushed the jn/foreigncall branch 2 times, most recently from d175911 to 5595287 Compare October 3, 2016 16:34

tkelman previously requested changes Oct 4, 2016

View reviewed changes

vtjnash force-pushed the jn/foreigncall branch from 5595287 to 373cabd Compare October 4, 2016 18:05

vtjnash force-pushed the jn/foreigncall branch 4 times, most recently from 39680e3 to cfe8efc Compare October 13, 2016 21:18

vtjnash added 15 commits January 19, 2017 14:58

eliminate ccall intrinsic

45efc24

add back syntax-deprecation warning for macros that were manually exp…

51cc949

…anding to Expr(:ccall)

some cleanup of runtime-intrinsics entry points

aafec21

entirely hookup runtime-intrinsics

4b18b8c

all intrinsics (but one) are now fully error-checking and runtime-interpreter-compatible

remove jl_has_intrinsics bandaid

a6a7463

only acquire the method lock at most once during codegen

43691c9

make ccall fully statically compilable

6dc5f50

and remove the ducttape!

eliminate uniqueness requirement on T_jlvalue (now rendered simply as…

b001294

… T_pint8)

temporarily make invalid ccall syntax only a warning

957d2cc

a handful of packages seem to have unused functions that are triggering this error

avoid BoUpSLP vectorizer InsertElementInst bug in llvm-3.3

37ac25d

remove unbox intrinsic

69b0f43

it was neutered by earlier commits, this just finishes deleting it

remove box intrinsic

56a1e4d

by adding type inference information to all of the intrinsics, the box intrinsic is no longer required for this purpose

remove unnecessary calls to box intrinsic

f0827da

rename box intrinsic to bitcast

497dbc9

this name better reflects what this intrinsic does, which is to change the type-tag on some data, without actually altering the data or its representation

add docs and news for ccall change

b34f47b

vtjnash force-pushed the jn/foreigncall branch from 7ebd07c to b34f47b Compare January 19, 2017 19:59

vtjnash merged commit 5112619 into master Jan 20, 2017

vtjnash deleted the jn/foreigncall branch January 20, 2017 05:01

maleadt added a commit to JuliaAttic/CUDArt.jl that referenced this pull request Feb 16, 2017

Disable unusable functions, now that ccall parses more strictly.

e846b2f

Ref JuliaLang/julia#18754

vchuravy pushed a commit to JuliaAttic/CUDArt.jl that referenced this pull request Feb 17, 2017

Disable unusable functions, now that ccall parses more strictly.

6ca68b3

Ref JuliaLang/julia#18754

rsrock mentioned this pull request Apr 6, 2017

Tag QuartzImageIO v0.3.0 [https://github.com/JuliaIO/QuartzImageIO.jl] JuliaLang/METADATA.jl#8717

Merged

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017

Add PR number and quotes to NEWS.md entry for JuliaLang#18754.

4c2c184

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017

Add PR number and quotes to NEWS.md entry for JuliaLang#18754.

5bad704

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 20, 2017

Add PR number and quotes to NEWS.md entry for JuliaLang#18754.

c9a8d33

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 20, 2017

Add PR number and quotes to NEWS.md entry for JuliaLang#18754.

001d40f

tkelman pushed a commit that referenced this pull request May 24, 2017

Add PR number and quotes to NEWS.md entry for #18754. (#21842)

e020772

tkelman pushed a commit that referenced this pull request Jun 3, 2017

Add PR number and quotes to NEWS.md entry for #18754. (#21842)

acf0c7c

(cherry picked from commit e020772)

tkelman mentioned this pull request Jul 13, 2017

Fix for windows and v0.6 jump-dev/CPLEX.jl#134

Merged

ViralBShah added the gpu Affects running Julia on a GPU label Sep 7, 2017


		.. _CUDA: http://llvm.org/docs/NVPTXUsage.html

		As with any ccall, it is essential to get the argument signature exactly correct.

Uh oh!

Conversation

vtjnash commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sacha0 Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maleadt commented Oct 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman commented Oct 3, 2016

Uh oh!

vtjnash commented Oct 3, 2016

Uh oh!

tkelman commented Oct 3, 2016

Uh oh!

vtjnash commented Oct 3, 2016

Uh oh!

tkelman commented Oct 3, 2016

Uh oh!

vtjnash commented Oct 4, 2016

Uh oh!

tkelman left a comment

Choose a reason for hiding this comment

Uh oh!

vtjnash commented Oct 4, 2016

Uh oh!

tkelman commented Oct 4, 2016

Uh oh!

vtjnash commented Oct 4, 2016

Uh oh!

tkelman commented Oct 4, 2016

Uh oh!

tkelman commented Oct 5, 2016

Uh oh!

vtjnash commented Oct 5, 2016

Uh oh!

JeffBezanson commented Oct 6, 2016

Uh oh!

vtjnash commented Oct 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

vtjnash commented Oct 1, 2016 •

edited

Loading

Sacha0 Oct 1, 2016 •

edited

Loading

tkelman Oct 3, 2016 •

edited

Loading