Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[NewOptimizer] Port #26826 to new optimizer #26981

Merged
merged 7 commits into from
May 6, 2018
Merged

[NewOptimizer] Port #26826 to new optimizer #26981

merged 7 commits into from
May 6, 2018

Conversation

jrevels
Copy link
Member

@jrevels jrevels commented May 4, 2018

No description provided.

jrevels
jrevels commented May 4, 2018
View reviewed changes
base/compiler/ssair/inlining2.jl Outdated
if isa(def, SSAValue) && is_tuple_call(ir, ir[def])
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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Keno
Keno reviewed May 4, 2018
View reviewed changes
base/compiler/ssair/inlining2.jl Outdated
@@ -577,15 +577,17 @@ function analyze_method!(idx, f, ft, metharg, methsp, method, stmt, atypes, sv,
end

# Handle vararg functions
prevararg_rewritten_atypes = atypes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ararslan ararslan added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label May 4, 2018
@jrevels
Copy link
Member Author

jrevels commented May 4, 2018

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

Ah, good point.

Keno
Keno approved these changes May 4, 2018
View reviewed changes
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Keno
Keno reviewed May 5, 2018
View reviewed changes
base/compiler/ssair/inlining2.jl
@@ -576,15 +576,6 @@ function analyze_method!(idx, f, ft, metharg, methsp, method, stmt, atypes, sv,
return ConstantCase(quoted(linfo.inferred_const), method, Any[methsp...], metharg)
end

# Handle vararg functions
isva = na > 0 && method.isva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isva is used below still.

Keno
Keno requested changes May 5, 2018
View reviewed changes
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jrevels
Copy link
Member Author

jrevels commented May 5, 2018

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
Copy link
Member

Keno commented May 5, 2018

Yes, tests pass on master.

@jrevels
Copy link
Member Author

jrevels commented May 5, 2018

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
Copy link
Member

Keno commented May 5, 2018

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

@Keno Keno mentioned this pull request May 5, 2018
Merged
@jrevels
Copy link
Member Author

jrevels commented May 5, 2018

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
Copy link
Member

Keno commented May 6, 2018

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

@Keno Keno merged commit 74af878 into master May 6, 2018
@vchuravy vchuravy deleted the jr/newoptvarargs branch May 7, 2018 01:20
@jrevels jrevels mentioned this pull request Jun 19, 2018
Closed
This was referenced Jul 7, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keno Keno Keno requested changes

Assignees
No one assigned
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrevels @Keno @ararslan