Inference for splatting numbers (and more) #27434

martinholters · 2018-06-05T08:11:36Z

Fixes #22291, closes #22292 (from where I've taken the tests).

But even more:

julia> using LinearAlgebra; foo(x) = (x...,); @code_warntype foo(qr(rand(3,3))) # master
Body::Tuple{Vararg{Union{Array{Float64,2}, LinearAlgebra.QRCompactWYQ{Float64,Array{Float64,2}}},N} where N}
1 1 ─ %1 = Core._apply(Core.tuple, %%x)::Tuple{Vararg{Union{Array{Float64,2}, LinearAlgebra.QRCompactWYQ{Float64,Array{Float64,2}}},N} where N}
  └──      return %1  

julia> using LinearAlgebra; foo(x) = (x...,); @code_warntype foo(qr(rand(3,3))) # this PR
Body::Tuple{LinearAlgebra.QRCompactWYQ{Float64,Array{Float64,2}},Array{Float64,2}}
1 1 ─ %1 = Core._apply(Core.tuple, %%x)::Tuple{LinearAlgebra.QRCompactWYQ{Float64,Array{Float64,2}},Array{Float64,2}}                   │
  └──      return %1

martinholters · 2018-06-05T12:31:43Z

Same error across the board:

PHI node has multiple entries for the same basic block with different incoming values!
  %value_phi7 = phi i64 [ %587, %L111 ], [ %588, %L111 ], [ %value_phi4, %L47.L112_crit_edge ]
label %L111
  %588 = load i64, i64* %29, align 8, !dbg !512983, !tbaa !8715
  %587 = load i64, i64* %29, align 8, !dbg !512983, !tbaa !8715
LLVM ERROR: Broken function found, compilation aborted!

This is interesting, because a) it somehow managed to build locally and b) even if inference gives a wrong result, bad things may happen, but invalid LLVM IR should not be among them AFAICT.

martinholters · 2018-06-06T07:14:54Z

Ok, a) resolved: make julia-debug fails locally, too. But regarding b), it's not the fault of the change to inference here, just the second commit, i.e.

diff --git a/base/number.jl b/base/number.jl
index 89cec5169e..514747bed3 100644
--- a/base/number.jl
+++ b/base/number.jl
@@ -233,7 +233,8 @@ julia> widemul(Float32(3.), 4.)
 """
 widemul(x::Number, y::Number) = widen(x)*widen(y)
 
-iterate(x::Number, done = false) = done ? nothing : (x, true)
+iterate(x::Number) = (x, true)
+iterate(x::Number, ::Any) = nothing
 isempty(x::Number) = false
 in(x::Number, y::Number) = x == y

is enough to make make julia-debug fail with above LLVM error. I'll try investigating.

martinholters · 2018-06-06T12:52:00Z

Can anyone give me a clue how I can figure out what method is being compiled when that error is thrown?

vtjnash · 2018-06-06T15:08:13Z

base/compiler/abstractinterpretation.jl

+            valtype = Any
+            break
        end
        if nounion.parameters[1] <: valtype && nounion.parameters[2] <: statetype


Copy this conditional above? (or at least the nounion.parameters[2] <: statetype part of it). If the new statetype is narrower, this must have been an infinite iterator (or throws an error).

True. However, for an infinite iterator, we get a slightly wider type then, e.g. Tuple{Char,Vararg{Char,N} where N} instead of Body::Tuple{Char,Char,Char,Char,Char,Char,Char,Char,Char,Char,Char,Char,Char,Vararg{Char,N} where N} for splatting Iterators.repeated('a'). Probably doesn't matter. Or is your suggestion to return Any[Bottom} (instead of breaking out of the loop) then? While theoretically correct (or did I get something wrong?), that looks rather aggressive.

vtjnash · 2018-06-06T15:09:37Z

base/number.jl

 widemul(x::Number, y::Number) = widen(x)*widen(y)

-iterate(x::Number, done = false) = done ? nothing : (x, true)
+iterate(x::Number) = (x, true)


Should never matter (after inference, inlining, or SROA), but constructing (x, nothing) would require 1 byte less space. Maybe use that instead?

Yes. I've actually changed that locally to see whether it makes a difference with the failure in make julia-debug (which it doesn't), and realized that it might be better anyway.

vtjnash · 2018-06-06T15:18:15Z

base/compiler/abstractinterpretation.jl


 # simulate iteration protocol on container type up to fixpoint
 function abstract_iteration(@nospecialize(itertype), vtypes::VarTable, sv::InferenceState)
    tm = _topmod(sv)


As implemented right now, this actually always uses Base, not _topmod(sv) to find iterate. We should probably redefine this as if isdefined(Main, :Base) || !isdefined(Main.Base, :iterate) || !isconst(Main.Base, :iterate) to be pedantic and precise about correctness here

Ok, will adjust then.

Um, are you sure? If I look at e.g. Core.Compiler.append_any(1), it seems to be calling Core.Compiler.iterate. And that's what we want to reflect, no?

I don’t think there’s any callers of Core.Compiler.append_any. The implementation of _apply right now always uses Base

martinholters · 2018-06-08T11:38:37Z

Ok, I've incorporated the comments locally (including returning Bottom for infinite iterators), but I'm struggling with the invalid LLVM IR when doing make julia-debug. I've narrowed it down to happen during jl_dump_native, and dump()ing the module, I find

; Function Attrs: sspstrong
define internal nonnull %jl_value_t addrspace(10)* @julia__unsafe_getindex_19848(%jl_value_t addrspace(10)* nonnull dereferenceable(32), i64
, %jl_value_t addrspace(10)* nonnull dereferenceable(40)) #11 !dbg !532598 {
top:
  %A = alloca %jl_value_t addrspace(10)*
  %I = alloca %jl_value_t addrspace(10)*
  %3 = alloca i64
  %4 = alloca i8
; ...
L111:                                             ; preds = %L110
  %587 = load i64, i64* %29, align 8, !dbg !532774, !tbaa !8717
  %588 = load i64, i64* %29, align 8, !dbg !532774, !tbaa !8717
  br i1 true, label %L112, label %L112, !dbg !532774

L112:                                             ; preds = %L111, %L111, %L47.L112_crit_edge
  %value_phi7 = phi i64 [ %587, %L111 ], [ %588, %L111 ], [ %value_phi4, %L47.L112_crit_edge ]
; ...
}
; ...

with this debug info (excerpt):

!4 = !{}
!8686 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !8687, size: 64, align: 64)
!8698 = !DIBasicType(name: "Int64", size: 64, encoding: DW_ATE_unsigned)
!8967 = !DIDerivedType(tag: DW_TAG_typedef, name: "Array", baseType: !8686)
!12765 = !{!8698}
!25612 = !DICompositeType(tag: DW_TAG_structure_type, name: "Tuple", size: 64, align: 64, elements: !12765,
                          runtimeLang: DW_LANG_Julia, identifier: "128")
!44905 = !{!8686, !8698, !25612}
!47125 = !{!8698, !8686}
!72478 = !DICompositeType(tag: DW_TAG_structure_type, name: "BitArray", size: 192, align: 64, elements: !44905,
                          runtimeLang: DW_LANG_Julia, identifier: "7968")
!117780 = !DICompositeType(tag: DW_TAG_structure_type, name: "#_unsafe_getindex", align: 8, elements: !4,
                           runtimeLang: DW_LANG_Julia, identifier: "7962")
!411431 = !DICompositeType(tag: DW_TAG_structure_type, name: "BitArray", size: 256, align: 64, elements: !411432,
                           runtimeLang: DW_LANG_Julia, identifier: "7969")
!411432 = !{!8686, !8698, !23573}
!532598 = distinct !DISubprogram(name: "_unsafe_getindex", linkageName: "julia__unsafe_getindex_19848",
                                 scope: null, file: !1846, line: 594, type: !532599, isLocal: false,
                                 isDefinition: true, scopeLine: 594, isOptimized: true, unit: !7664,
                                 variables: !532601)
!532599 = !DISubroutineType(types: !532600)
!532600 = !{!72478, !411431, !8698, !8967}
!532601 = !{!532602, !532603, !532604}
!532602 = !DILocalVariable(name: "#self#", arg: 1, scope: !532598, file: !1846, line: 594, type: !117780)
!532603 = !DILocalVariable(name: "A", arg: 3, scope: !532598, file: !1846, line: 594, type: !411431)
!532604 = !DILocalVariable(name: "I...", arg: 4, scope: !532598, file: !1846, line: 594, type: !532605)
!532605 = !DICompositeType(tag: DW_TAG_structure_type, name: "Tuple", size: 128, align: 64, elements: !47125,
                           runtimeLang: DW_LANG_Julia, identifier: "36316")

How do I figure out for which types this was specialized exactly?

martinholters · 2018-06-18T10:01:29Z

Ok, with #27609 in, this now seems to pass CI (Circle 64bit was killed (OOM?), Travis on OSX was hit by #26725). I've also addressed Jameson's remarks, so unless @nanosoldier runbenchmarks(ALL, vs=":master") disagrees, this should be good to go.

nanosoldier · 2018-06-18T16:16:04Z

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

Exploit the fact that the (minimum) number of elements obtained from iteration can be derived from the types in certain cases and hence, a more exact type can be inferred for splatting them.

Letting the presence of a second argument alone decide whether iteration is done lets `abstract_iteration` determine that splatting a `Number` yields exactly one `Number`.

martinholters · 2018-06-26T07:02:49Z

As I let this go a bit stale, @nanosoldier runbenchmarks(ALL, vs=":master") once more, and if everything looks ok, I'll merge.

nanosoldier · 2018-06-26T14:16:48Z

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

martinholters · 2018-06-26T14:45:02Z

The logical next step would be eliding the call to _apply in more cases, i.e. when all involved iterables are inferred to result in a fixed length (up to some limit, probably). From what I understand, that would mean reworking the condition at

julia/base/compiler/ssair/inlining.jl

Lines 793 to 794 in 2e168b5

    
           # TODO: We could basically run the iteration protocol here 
        
           if !isa(typ, DataType) || typ.name !== Tuple.name || isvatuple(typ)

and handling the respective case with unrolled iterate calls in

julia/base/compiler/ssair/inlining.jl

Line 556 in 2e168b5

    
           function rewrite_apply_exprargs!(ir::IRCode, idx::Int, argexprs::Vector{Any}, sv::OptimizationState)

But I'm relatively clueless how to thread the information gathered during inference through to the inlining pass. If @Keno or @vtjnash can give me a hint, I might try to give it a shot.

martinholters added the compiler:inference Type inference label Jun 5, 2018

vtjnash reviewed Jun 6, 2018

View reviewed changes

martinholters mentioned this pull request Jun 15, 2018

Invalid LLVM IR for certain iteration pattern #27594

Closed

martinholters force-pushed the mh/abstract_iteration branch from a69650f to df14e50 Compare June 18, 2018 06:38

martinholters added 4 commits June 26, 2018 08:42

Improve abstract_iteration

fd36a48

Exploit the fact that the (minimum) number of elements obtained from iteration can be derived from the types in certain cases and hence, a more exact type can be inferred for splatting them.

Let splatting of infinite iterators be inferred as Bottom

eafefa3

Use Base.iterate instead of topmod().iterate in abstract_iteration

78b9e89

Make iterate(::Number) more digestible by abstract_iteration

e086018

Letting the presence of a second argument alone decide whether iteration is done lets `abstract_iteration` determine that splatting a `Number` yields exactly one `Number`.

martinholters force-pushed the mh/abstract_iteration branch from df14e50 to e086018 Compare June 26, 2018 07:01

martinholters merged commit 2e168b5 into master Jun 26, 2018

martinholters deleted the mh/abstract_iteration branch June 26, 2018 14:21

andyferris mentioned this pull request Jun 27, 2018

Bump tuple inference length cutoff from 16 to 32 #27398

Merged

martinholters mentioned this pull request Sep 5, 2018

work around a splatting penalty in twiceprecision #29060

Merged

This was referenced Sep 7, 2018

Serious regression of warp! JuliaImages/ImageTransformations.jl#60

Closed

Performance problems of Julia #29078

Closed

KristofferC mentioned this pull request Sep 10, 2018

Performance of splatting a number #29114

Closed

martinholters mentioned this pull request Dec 21, 2018

make inference of _apply match the implementation more closely #30483

Merged

Uh oh!

Inference for splatting numbers (and more) #27434

Inference for splatting numbers (and more) #27434

Uh oh!

Conversation

martinholters commented Jun 5, 2018

Uh oh!

martinholters commented Jun 5, 2018

Uh oh!

martinholters commented Jun 6, 2018

Uh oh!

martinholters commented Jun 6, 2018

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!

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!

martinholters commented Jun 8, 2018

Uh oh!

martinholters commented Jun 18, 2018

Uh oh!

nanosoldier commented Jun 18, 2018

Uh oh!

martinholters commented Jun 26, 2018

Uh oh!

nanosoldier commented Jun 26, 2018

Uh oh!

martinholters commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants