Type inference free comprehensions #11398

carnaval · 2015-05-22T01:47:16Z

The first part of this is moving lowering of comprehensions to a julia macro. I think it's a good thing in itself.

The second part is to make comprehension have the same semantic as map that is :

remove type_goto and static_typeof
use typejoin to compute the array element type
be optimistic about the first element type

This implies that we get Vector{Union()} for 0-length input, as was discussed somewhere else.

The cool things :

the array type is the same in the repl & in functions, it only depends on the computation not inference. It makes this possible :

julia> [rand() > 0.01 ? 1 : "a" for i=1:4, j=1:4]
4x4 Array{Int64,2}:
 1  1  1  1
 1  1  1  1
 1  1  1  1
 1  1  1  1

julia> [rand() > 0.01 ? 1 : "a" for i=1:4, j=1:4]
4x4 Array{Any,2}:
 1  1  1  1   
 1  1  1   "a"
 1  1  1  1   
 1  1  1   "a"

array type is still inferred correctly, with the change that now the result is Union(Array{Union()},Array{T}), but it's ok when you index in it as demonstrated by :

julia> f() = (A = [1.0*i for i=1:4]; A[1])
f (generic function with 1 method)
julia> Base.return_types(f,())[1]
Float64

in cases where inference gets it right, this should be close to zero cost since the pessimistic codepath gets optimized away. Removing the Vector{Union()} after one index call is a bit harder since in general it requires backward dataflow analysis. I didn't check and the macro may be emitting ugly things for now so it may need a bit more work.

The code of the macro is ugly. I've also not yet gone through the frontend to remove dead comprehension lowering code. The dict comprehension case should probably also be written. Sysimg code was also a bit butchered in the process to get it work, we would need to go back and make it right. Thanks to @jakebolewski who was easily baited into doing this.

All in all, not close to done but enough for me to get a type_goto free base for now.

blakejohnson · 2015-05-22T01:56:37Z

base/expr.jl

@@ -39,7 +37,7 @@ copy(s::SymbolNode) = SymbolNode(s.name, s.typ)

 # copy parts of an AST that the compiler mutates
 astcopy(x::Union(SymbolNode,Expr)) = copy(x)
-astcopy(x::Array{Any,1}) = Any[astcopy(a) for a in x]
+astcopy(x::Array{Any,1}) = map(a->astcopy(a),x)


maybe just map(astcopy, x)?

sure. all of this is a mess and I'll go back to sort it out.

carnaval · 2015-05-22T17:22:52Z

Added the dict stuff (works the same way) and cleaned up the dead code from inference & frontend (this PR has a negative line count total).

The generated code for the untyped version of [X for Y1 in Z1, Y2 in Z2, ...] looks like :

index = 1
len = length(Z1)*...
if len == 0
   return Array(Union(), 0)
end
s1 = start(Z1)
Y1,s1 = next(Z1)
s2 = start(Z2)
Y2,s2 = next(Z2)
...
v = X
T = typeof(v)
result = Array(T, len)
@goto inner_loop
while !done(Z1,s1)
   Y1,s1 = next(Z1,s1)
   while !done(Z2,s2)
      Y2,s2 = next(Z2,s2)
      ...
      v = X
      S = typeof(v)
      if !(S <: T)
         T = typejoin(T,S)
         result_next = Array(T, len)
         copy!(result_next, 1, result, 1, index-1)
         result = result_next
      end
      @label inner_loop
      result[index] = v
      index += 1
   end
end

carnaval · 2015-05-22T23:34:18Z

This now supports colon in comprehension correctly. The current master one is buggy for more than one colon. This now works :

[[1 2; 3 4] for :, :] == [1 2; 3 4]

and any combination of typed/untyped comprehension with colon/non-colon indices also should. The rule being :

Any colon will be substituted by an implicit index into the result of the comprehension expression. The size of this slice is determined by the size of the corresponding slice in the first result.

There is no error checking if you input an expression which changes size (same as on master), also the expression will be recomputed for each iteration of course. This feature seems very underused, but now it should work correctly :-)

- Move lowering of the comprehension to a julia macro - Use typejoin to compute the array/dict type while being optimistic about the first element - Fix comprehension colon logic - Make tests pass

carnaval · 2015-05-27T23:48:18Z

Well my job here is "done". Tests pass with some slight changes here and there. I'm not advocating for merge since I'm sure there are some perf regression everywhere due to the type instability. It is useful to me as is so if no one is interested I'm just gonna leave the PR hanging here for now.

JeffBezanson · 2015-05-28T00:55:19Z

base/essentials.jl

@@ -135,7 +135,13 @@ function length_checked_equal(args...)
    n
 end

-map(f::Function, a::Array{Any,1}) = Any[ f(a[i]) for i=1:length(a) ]
+function map(f::Function, a::Array{Any,1})


Is this rewrite necessary? If so, why?

The comprehension macro is not defined yet, so if I remember correctly this was an issue during bootstrap (one of the functions in the macro indirectly called this method, I don't know if that is still the case).

Ah, of course.

JeffBezanson · 2015-05-28T01:03:12Z

This is really cool! Awesome work. I think we can seriously consider merging this. Although it would be nice to also base comprehensions on map, but that's definitely work for a future release.

tkelman · 2015-05-28T05:31:12Z

base/inference.jl

@@ -2,7 +2,7 @@

 # parameters limiting potentially-infinite types
 const MAX_TYPEUNION_LEN = 3
-const MAX_TYPE_DEPTH = 4
+const MAX_TYPE_DEPTH = 6


sneaky... ref #10230

StefanKarpinski · 2015-06-17T14:30:57Z

Is there anything preventing us from merging this?

StefanKarpinski · 2015-06-17T14:31:30Z

Aside from the need to rebase due to some (hopefully) minor conflicts?

yuyichao · 2015-06-17T14:33:50Z

I think @carnaval was worrying about type stability caused by type instability caused by Array{Union()}. I'm also wondering what will happen when there are many Array{Union()} passing around....

StefanKarpinski · 2015-06-17T14:42:30Z

@JeffBezanson, any thoughts?

yuyichao · 2015-06-17T14:52:33Z

And here's @carnaval's comment #7258 (comment).

My concern is largely based on #11658 (which can be fixed in general but probably require more polishing than what we want to wait for 0.4)

StefanKarpinski · 2015-06-17T14:53:56Z

Ok, yeah that's too bad. Maybe this belongs in Arraymageddon anyway.

vtjnash · 2016-07-14T17:45:07Z

anything from here still useful?

blakejohnson reviewed May 22, 2015
View reviewed changes

carnaval force-pushed the ob/goodbye-type_goto branch from 684baff to 6553684 Compare May 22, 2015 17:01

carnaval force-pushed the ob/goodbye-type_goto branch 2 times, most recently from e3106b1 to 238faa7 Compare May 22, 2015 23:33

carnaval force-pushed the ob/goodbye-type_goto branch from 238faa7 to 932094e Compare May 24, 2015 20:19

Type inference free comprehensions

c4c6817

- Move lowering of the comprehension to a julia macro - Use typejoin to compute the array/dict type while being optimistic about the first element - Fix comprehension colon logic - Make tests pass

carnaval force-pushed the ob/goodbye-type_goto branch from f744dbc to c4c6817 Compare May 25, 2015 05:10

carnaval added 2 commits May 27, 2015 12:45

temp things for gf

50cefae

fix copy(::Array{⊥})

80191b9

JeffBezanson reviewed May 28, 2015
View reviewed changes

tkelman reviewed May 28, 2015
View reviewed changes

JeffBezanson mentioned this pull request Jan 24, 2016

WIP: redesign comprehensions #14782

Closed

3 tasks

JeffBezanson closed this Sep 12, 2016

DilumAluthge deleted the ob/goodbye-type_goto branch March 25, 2021 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type inference free comprehensions #11398

Type inference free comprehensions #11398

carnaval commented May 22, 2015

blakejohnson May 22, 2015

carnaval May 22, 2015

carnaval commented May 22, 2015

carnaval commented May 22, 2015

carnaval commented May 27, 2015

JeffBezanson May 28, 2015

jakebolewski May 28, 2015

JeffBezanson May 28, 2015

JeffBezanson commented May 28, 2015

tkelman May 28, 2015

StefanKarpinski commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

yuyichao commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

yuyichao commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

vtjnash commented Jul 14, 2016

Type inference free comprehensions #11398

Type inference free comprehensions #11398

Conversation

carnaval commented May 22, 2015

blakejohnson May 22, 2015

Choose a reason for hiding this comment

carnaval May 22, 2015

Choose a reason for hiding this comment

carnaval commented May 22, 2015

carnaval commented May 22, 2015

carnaval commented May 27, 2015

JeffBezanson May 28, 2015

Choose a reason for hiding this comment

jakebolewski May 28, 2015

Choose a reason for hiding this comment

JeffBezanson May 28, 2015

Choose a reason for hiding this comment

JeffBezanson commented May 28, 2015

tkelman May 28, 2015

Choose a reason for hiding this comment

StefanKarpinski commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

yuyichao commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

yuyichao commented Jun 17, 2015

StefanKarpinski commented Jun 17, 2015

vtjnash commented Jul 14, 2016