RFC: replace `ANY` with `@nospecialize` annotation #22666

JeffBezanson · 2017-07-03T20:34:25Z

Part of #11339.

This provides a much nicer and more general approach to specialization annotations, that stays out of the way of the type system.

TODO:

NEWS and doc updates
Add deprecation message
Replace uses of ANY in Base

tkelman · 2017-07-03T20:50:49Z

base/exports.jl

@@ -1285,6 +1285,7 @@ export
    @simd,
    @inline,
    @noinline,
+    @nospecialize,


add to stdlib doc index

Remove from exports? Or maybe export from Base.Meta? Seems special-purpose enough that it doesn't need to be in the default namespace.

tkelman · 2017-07-03T20:52:21Z

test/meta.jl

@@ -144,3 +144,14 @@ baremodule B
 end
 @test B.x == 3
 @test B.M.x == 4
+
+# specialization annotations
+function _nospec_some_args(@nospecialize(x), y, @nospecialize z::Int)


is it redundant here on the z arg that has a concrete type constraint?

Pretty much; it will just have no effect in that case.

iamed2 · 2017-07-03T23:48:12Z

Is there any functional difference between this and ANY?

JeffBezanson · 2017-07-04T04:33:40Z

No. But it will allow additional functionality in the future, such as @nospecialize x::AbstractArray, where the type and specialization are de-coupled.

tkelman · 2017-07-04T05:41:43Z

src/julia.h

@@ -255,6 +255,7 @@ typedef struct _jl_method_t {

    int32_t nargs;
    int32_t called;  // bit flags: whether each of the first 8 arguments is called
+    int32_t nospec;  // bit flags: which arguments should not be specialized


how many arguments can this use before it breaks down? will there be a sane error or warning with 32 or more arguments?

I'll add a warning.

andyferris · 2017-07-04T10:32:10Z

Very nice - I like this a lot!

tkelman · 2017-07-05T06:38:18Z

src/method.c

+        jl_value_t *ti = jl_svecref(atypes, i);
+        if (ti == jl_ANY_flag ||
+            (jl_is_vararg_type(ti) && jl_tparam0(jl_unwrap_unionall(ti)) == jl_ANY_flag)) {
+            jl_depwarn("`x::ANY` is deprecated, use `@nospecialize(x)` instead.",


add note in deprecated.jl

tkelman · 2017-07-05T16:27:28Z

test/broadcast.jl

@@ -346,7 +346,7 @@ end
 let f17314 = x -> x < 0 ? false : x
    @test eltype(broadcast(f17314, 1:3)) === Int
    @test eltype(broadcast(f17314, -1:1)) === Integer
-    @test eltype(broadcast(f17314, Int[])) === Union{Bool,Int}
+    @test eltype(broadcast(f17314, Int[])) == Union{Bool,Int}


was this a bug or?

It's a bug in the test. The type might be Union{Bool,Int} or Union{Int,Bool}, which have different structures that can be distinguished by ===. The show method for unions sorts the types though, obscuring this. #22664 is about normalizing union types so this doesn't come up.

and changing ANY to nospecialize triggers a different result more often somehow?

Yes, that seems to be the case. I'm not sure it's worth the time to track down why.

tkelman · 2017-07-06T21:06:10Z

base/deprecated.jl

@@ -1519,6 +1519,8 @@ function replace(s::AbstractString, pat, f, n::Integer)
    end
 end

+# ::ANY is deprecated in src/method.c


and jl_ANY_flag should be removed everywhere else in src, right?

tkelman · 2017-07-06T21:09:20Z

src/subtype.c

@@ -790,8 +790,6 @@ static int forall_exists_equal(jl_value_t *x, jl_value_t *y, jl_stenv_t *e);
 // diagonal rule (record_var_occurrence).
 static int subtype(jl_value_t *x, jl_value_t *y, jl_stenv_t *e, int param)
 {
-    if (x == jl_ANY_flag) x = (jl_value_t*)jl_any_type;
-    if (y == jl_ANY_flag) y = (jl_value_t*)jl_any_type;


should these wait until the deprecation is removed?

I modified the method definition code to remove ANY from signatures before the type system sees them, so this should be ok. Using ANY outside a method signature is not allowed.

tkelman · 2017-07-09T13:48:27Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-07-09T18:06:50Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

timholy · 2017-07-12T12:54:22Z

FYI there are a couple new ANYs in boot.jl.

vtjnash

This looks good, just minor comments to fix.

vtjnash · 2017-07-12T18:51:27Z

base/boot.jl

+ccall(:jl_toplevel_eval_in, Any, (Any, Any),
+      Core, quote
+      Expr(args...) = ($(_expr(:meta,:nospecialize,:args)); _expr(args...))
+      end)


might be simpler to define @nospecialize as _expr(:meta, :nospecialize, x), then I think we can avoid the need to unroll this as much (since it can just use the macro).

vtjnash · 2017-07-12T18:55:06Z

base/essentials.jl

@@ -17,6 +17,38 @@ end
 macro _noinline_meta()
    Expr(:meta, :noinline)
 end
+"""


put empty newlines between functions

vtjnash · 2017-07-12T18:57:33Z

base/essentials.jl

+should not be specialized for different types of that argument.
+This is only a hint for avoiding excess code generation.
+Can be applied to an argument within a formal argument list, or in the
+function body.


Not sure we want to encourage the latter form, but it should be mentioned that it should be the first statement (and must be a statement), or we might not scan it.

vtjnash · 2017-07-12T18:58:57Z

base/exports.jl

@@ -1285,6 +1285,7 @@ export
    @simd,
    @inline,
    @noinline,
+    @nospecialize,


Remove from exports? Or maybe export from Base.Meta? Seems special-purpose enough that it doesn't need to be in the default namespace.

vtjnash · 2017-07-12T19:06:30Z

src/jltypes.c

@@ -1937,10 +1933,11 @@ void jl_init_types(void)
                            "invokes",
                            "nargs",
                            "called",
+                            "nospec",


no abbreviations. perhaps call this nospecialize?

vtjnash · 2017-07-12T19:12:44Z

src/method.c

+                            jl_printf(JL_STDERR,
+                                      "WARNING: @nospecialize annotation applied to a non-argument.\n");
+                        }
+                        else if (sn >= sizeof(m->nospec)*8) {


spaces around binary operator

vtjnash · 2017-07-12T19:13:19Z

src/method.c

+                    if (sn >= 0) {  // @nospecialize on self is valid but currently ignored
+                        if (sn > (m->nargs - 2)) {
+                            jl_printf(JL_STDERR,
+                                      "WARNING: @nospecialize annotation applied to a non-argument.\n");


make this an error

vtjnash · 2017-07-12T19:14:52Z

src/gf.c

@@ -611,8 +611,8 @@ static void jl_cacheable_sig(

        int notcalled_func = (i > 0 && i <= 8 && !(definition->called & (1 << (i - 1))) &&
                              jl_subtype(elt, (jl_value_t*)jl_function_type));
-        if (decl_i == jl_ANY_flag) {
-            // don't specialize on slots marked ANY
+        if (i > 0 && i <= 8 && (definition->nospec & (1 << (i - 1))) &&


i <= sizeof(m->nospec) * 8

vtjnash · 2017-07-12T19:23:50Z

src/ast.scm

@@ -310,6 +318,9 @@
 (define (kwarg? e)
  (and (pair? e) (eq? (car e) 'kw)))

+(define (nospecialize-meta? e)
+  (and (length> e 2) (eq? (car e) 'meta) (eq? (cadr e) 'nospecialize)))


add another parameter one::Bool and check (if one (eq? (length e) 3) (length> e 2))

vtjnash · 2017-07-12T19:27:22Z

src/julia-syntax.scm

@@ -3780,6 +3796,9 @@ f(x) = yt(x)
          ((and (pair? e) (eq? (car e) 'outerref))
           (let ((idx (get sp-table (cadr e) #f)))
                (if idx `(static_parameter ,idx) (cadr e))))
+          ((and (length> e 2) (eq? (car e) 'meta) (eq? (cadr e) 'nospecialize))


((nospecialize-meta e #f)

JeffBezanson · 2017-07-12T21:24:58Z

Thanks. All comments addressed.

tkelman · 2017-07-13T12:12:50Z

base/essentials.jl

@@ -17,6 +17,42 @@ end
 macro _noinline_meta()
    Expr(:meta, :noinline)
 end
+
+"""
+    @nospecialize


I wonder whether it should be documented as Base.@nospecialize ?

Actually, not having it exported was pretty annoying. That's partly just an artifact of Base and tests using this more heavily than most code, but (1) other annotations like @inline are exported, and (2) it's hard to imagine @nospecialize conflicting with anything. So I'm more inclined to just export it.

tkelman · 2017-07-13T12:15:47Z

base/inference.jl

    idx = reinterpret(Int32, f) + 1
    t_ifunc[idx] = (minarg, maxarg, tfunc)
    t_ifunc_cost[idx] = cost
 end
-function add_tfunc(#=@nospecialize::Builtin=# f::Function, minarg::Int, maxarg::Int, tfunc::ANY, cost::Int)
+function add_tfunc(#=@nospecialize::Builtin=# f::Function, minarg::Int, maxarg::Int, @nospecialize(tfunc), cost::Int)


was the #=@nospecialize::Builtin=# left as a todo for this pr to address?

vtjnash · 2017-07-13T21:29:48Z

base/boot.jl

+ccall(:jl_toplevel_eval_in, Any, (Any, Any),
+      Core, quote
+      (f::typeof(Typeof))(x) = ($(_expr(:meta,:nospecialize,:x)); isa(x,Type) ? Type{x} : typeof(x))
+      end)


why does this still need the more complex form? Also, why does it need f::typeof(Typeof) – could we just use Typeof(@nospecialize x) = ...

Because Typeof itself is used to lower method definitions not of the form (f::T)(...) = ....

otherwise we can run into `Union{Bool,Int} !== Union{Int,Bool}`

JeffBezanson added the deprecation This change introduces or involves a deprecation label Jul 3, 2017

JeffBezanson force-pushed the jb/ANY branch from 384a474 to fc7419d Compare July 3, 2017 20:49

tkelman reviewed Jul 3, 2017

View reviewed changes

ararslan added the types and dispatch Types, subtyping and method dispatch label Jul 3, 2017

JeffBezanson force-pushed the jb/ANY branch from fc7419d to 24dec72 Compare July 4, 2017 05:01

tkelman reviewed Jul 4, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from 24dec72 to 8f07a28 Compare July 5, 2017 06:30

tkelman reviewed Jul 5, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from 8f07a28 to cbddec2 Compare July 5, 2017 16:24

tkelman reviewed Jul 5, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from cbddec2 to 6494b2a Compare July 6, 2017 16:07

tkelman reviewed Jul 6, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch 2 times, most recently from 43643d2 to bcbfefb Compare July 8, 2017 18:11

JeffBezanson force-pushed the jb/ANY branch 2 times, most recently from b481326 to fc8d192 Compare July 10, 2017 21:54

vtjnash reviewed Jul 12, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from fc8d192 to 6a74f93 Compare July 12, 2017 21:24

JeffBezanson force-pushed the jb/ANY branch from 6a74f93 to 215902a Compare July 13, 2017 00:12

tkelman reviewed Jul 13, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from 215902a to a53afd9 Compare July 13, 2017 19:52

vtjnash reviewed Jul 13, 2017

View reviewed changes

JeffBezanson force-pushed the jb/ANY branch from a53afd9 to e7240fd Compare July 14, 2017 02:40

JeffBezanson added 2 commits July 14, 2017 00:20

use == to compare to Union{Bool,Int} in broadcast test

3637320

otherwise we can run into `Union{Bool,Int} !== Union{Int,Bool}`

replace ANY with @nospecialize annotation. part of #11339

9d00563

JeffBezanson force-pushed the jb/ANY branch from e7240fd to 9d00563 Compare July 14, 2017 04:21

tkelman merged commit 6b9d2c6 into master Jul 14, 2017

tkelman deleted the jb/ANY branch July 14, 2017 13:47

RFC: replace ANY with @nospecialize annotation #22666

RFC: replace ANY with @nospecialize annotation #22666

Conversation

JeffBezanson commented Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamed2 commented Jul 3, 2017

JeffBezanson commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 9, 2017

nanosoldier commented Jul 9, 2017

timholy commented Jul 12, 2017

vtjnash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jul 12, 2017

Choose a reason for hiding this comment

JeffBezanson Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RFC: replace `ANY` with `@nospecialize` annotation #22666

RFC: replace `ANY` with `@nospecialize` annotation #22666

JeffBezanson commented Jul 3, 2017 •

edited

Loading

JeffBezanson Jul 13, 2017 •

edited

Loading