[NewOptimizer] Port #26826 to new optimizer by jrevels · Pull Request #26981 · JuliaLang/julia

jrevels · 2018-05-04T18:23:25Z

No description provided.

jrevels · 2018-05-04T18:24:34Z

base/compiler/ssair/inlining2.jl

                    for tuparg in ir[def].args[2:end]
                        push!(new_atypes, exprtype(tuparg, ir, ir.mod))
                    end
+                elseif (isa(def, Argument) && def === stmt.args[end] && !isempty(sv.result_vargs))


@Keno I don't think we need a new field in IRCode for this, since !isempty(sv.result_vargs)) will only be true if it's a varargs method AND it has cached varargs type info (IIUC).

Oh, I forgot to check def.n. Let me do that...

Keno · 2018-05-04T19:09:13Z

base/compiler/ssair/inlining2.jl

    end

    # Handle vararg functions
+    prevararg_rewritten_atypes = atypes


If this is what's used for the cache, is the below atypes actually used anywhere?

Oh neat, you're right - the modified atypes was used elsewhere in the old inlining code, but not here. I guess I can just remove that code then.

jrevels · 2018-05-04T20:05:09Z

Seems like f(args...) = g(args..., 1) could benefit from this as well.

Ah, good point.

Keno

LGTM if tests pass with this and the new optimizer enabled.

Keno · 2018-05-05T00:06:53Z

base/compiler/ssair/inlining2.jl

    end

-    # Handle vararg functions
-    isva = na > 0 && method.isva


isva is used below still.

Keno

A bunch of thing seems to go of the rails when I run the tests with this PR.

jrevels · 2018-05-05T01:13:20Z

A bunch of thing seems to go of the rails when I run the tests with this PR.

Just started a local build + tests with the new optimizer enabled, anything I should watch out for? Do all tests on master pass with the new optimizer enabled (besides the ones from #26826, which should require this port)?

Keno · 2018-05-05T01:15:06Z

Yes, tests pass on master.

jrevels · 2018-05-05T02:12:35Z

Okay, I ran tests locally, and it seems there's only one thing that's failing:

SparseArrays/higherorderfns: Test Failed at /Users/jarrettrevels/data/repos/julia-dev/usr/share/julia/stdlib/v0.7/SparseArrays/test/higherorderfns.jl:269
  Expression: #= /Users/jarrettrevels/data/repos/julia-dev/usr/share/julia/stdlib/v0.7/SparseArrays/test/higherorderfns.jl:269 =# @allocated(broadcast!(f, Q, X, Y, Z)) == 0
   Evaluated: 16 == 0

This is the broken sparse arrays broadcast test that #26826 allowed us to mark as unbroken. I guess it's still broken with the new optimizer. We could investigate why, and in the meantime just mark it @test_broken like it was before #26826.

Keno · 2018-05-05T02:14:01Z

Is that with this PR merged? That's what I saw before I merged this PR.

jrevels · 2018-05-05T14:06:07Z

That's from running make test on 7506dc1 with

diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl
index 5943037..73fba2c 100644
--- a/base/compiler/optimize.jl
+++ b/base/compiler/optimize.jl
@@ -261,7 +261,7 @@ function isinlineable(m::Method, src::CodeInfo, mod::Module, params::Params, bon
     return inlineable
 end

-const enable_new_optimizer = RefValue(false)
+const enable_new_optimizer = RefValue(true)

 # converge the optimization work
 function optimize(me::InferenceState)

I could run it on the merge commit as well, did that change things (i.e. have changes to master since 429a885 broken other tests w.r.t. the new optimizer when combined with this PR)?

A summary for anybody following along (cc @vchuravy, since you asked about the relevant test in #26989):

#26826 added some compiler tests that should've failed under the new optimizer before this PR, and this PR allows those tests to pass w/ the new optimizer enabled. Furthermore, #26826 fixed the aforementioned @test_broken sparse broadcast test as a side effect, so #26826 changed it from @test_broken to @test. However, it seems that this test is still broken under the new optimizer, even with this PR.

Keno · 2018-05-06T22:13:33Z

Seems like I may have had other thing locally mess up my test results. Merging.

jrevels added 2 commits May 4, 2018 11:05

fix InferenceResult cache lookup for new optimizer

fa83a42

utilize cached vararg type info in new inlining pass

4aeeccf

jrevels commented May 4, 2018

View reviewed changes

fix test to work with new optimizer

e4c90a4

jrevels force-pushed the jr/newoptvarargs branch from dd2729f to e4c90a4 Compare May 4, 2018 18:31

Keno reviewed May 4, 2018

View reviewed changes

fix predicate for exploiting cached varargs type info

6e78ef1

ararslan added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label May 4, 2018

atypes no longer needs to be modified to match cached types

7f13234

Keno approved these changes May 4, 2018

View reviewed changes

don't require varargs to be in last position to exploit cached type info

254ff39

jrevels force-pushed the jr/newoptvarargs branch from b7bec8d to 254ff39 Compare May 4, 2018 20:14

Keno reviewed May 5, 2018

View reviewed changes

Keno requested changes May 5, 2018

View reviewed changes

restore missing isva boolean

7506dc1

Keno mentioned this pull request May 5, 2018

[GCLowering] Fix phis of unions #26989

Merged

Keno merged commit 74af878 into master May 6, 2018

vchuravy deleted the jr/newoptvarargs branch May 7, 2018 01:20

jrevels mentioned this pull request Jun 19, 2018

bad inferrability is back on recent Julia master JuliaLabs/Cassette.jl#45

Closed

This was referenced Jul 7, 2018

Cassette Compatibility #25492

Closed

poor inference when calling constant IntrinsicFunction passed as argument #28003

Closed

Uh oh!

Conversation

jrevels commented May 4, 2018

Uh oh!

jrevels May 4, 2018

Choose a reason for hiding this comment

Uh oh!

jrevels May 4, 2018

Choose a reason for hiding this comment

Uh oh!

Keno May 4, 2018

Choose a reason for hiding this comment

Uh oh!

jrevels May 4, 2018

Choose a reason for hiding this comment

Uh oh!

jrevels commented May 4, 2018

Uh oh!

Keno left a comment

Choose a reason for hiding this comment

Uh oh!

Keno May 5, 2018

Choose a reason for hiding this comment

Uh oh!

Keno left a comment

Choose a reason for hiding this comment

Uh oh!

jrevels commented May 5, 2018

Uh oh!

Keno commented May 5, 2018

Uh oh!

jrevels commented May 5, 2018

Uh oh!

Keno commented May 5, 2018

Uh oh!

jrevels commented May 5, 2018

Uh oh!

Keno commented May 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants