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.

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

add a lattice element type for constants in inference #15785

Merged
merged 2 commits into from
Apr 19, 2016
Merged

Conversation

JeffBezanson
Copy link
Member

This incorporates a bit of constant propagation into type inference, giving better type information and removing some special-case code we'd started accumulating.

Todo:

  • Get tests passing, make sure no performance or type info regressions
  • Take advantage of the extra information in more t-functions

@JeffBezanson JeffBezanson force-pushed the jb/constantprop branch 4 times, most recently from 037e22e to 922dcb5 Compare April 8, 2016 19:06
@JeffBezanson
Copy link
Member Author

runbenchmarks(ALL, vs = "JuliaLang/julia:master")

@nanosoldier
Copy link
Collaborator

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

@@ -1345,11 +1254,33 @@ function type_too_complex(t::ANY, d)
return false
end

## lattice operators

function ⊑(a::ANY, b::ANY)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this reserve this unicode operator so packages can no longer use it?

Copy link
Member

Choose a reason for hiding this comment

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

yes, but only for packages that start with import Core.Inference.⊑ :P

@JeffBezanson
Copy link
Member Author

Anybody have opinions on the benchmark results? Worst number is 2.67x allocations on a sorting benchmark, which I can't reproduce locally. Another one has 0.03x allocations, which is pretty implausible.

@KristofferC
Copy link
Member

I've seen all of those benchmarks in previous reports. They seem to be a bit wonky.

@vtjnash
Copy link
Member

vtjnash commented Apr 10, 2016

that does look strange. i think you may need to manually review the core.jl tests and the benchmarks, to make sure that constant propagation won't change what's being tested.

@JeffBezanson
Copy link
Member Author

I think we'll only really need to worry if and when we can infer the type of Any[x][1].

@vtjnash
Copy link
Member

vtjnash commented Apr 10, 2016

just browsing at random: the test for issue #4688, may be assuming that true == true is not inferrable, the test for issue #10281 appears to assume that if false is not DCE before codegen.

@JeffBezanson
Copy link
Member Author

The test for #4688 looks bit-rotted at this point; it seems to be related to whether x was a captured variable in the old implementation of closures.

For #10281, we have been able to infer if false for quite some time. In fact the test might be depending on that.

@jrevels
Copy link
Member

jrevels commented Apr 11, 2016

All the benchmark changes reported here are known to be noisy (including the weird allocation counts reported by the sorting benchmarks). If they can't be reproduced locally, then you can chalk it up to noise.

@JeffBezanson JeffBezanson changed the title WIP: add a lattice element type for constants in inference add a lattice element type for constants in inference Apr 13, 2016
if isType(s)
s = s.parameters[1]
elseif isa(s,Const)
s = typeof(s.val)
Copy link
Member

Choose a reason for hiding this comment

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

i think you would want s = s here, to preserve the type identity

@JeffBezanson
Copy link
Member Author

Comments addressed.

@vtjnash
Copy link
Member

vtjnash commented Apr 17, 2016

hm, github isn't showing quite what I hoped for a diff here – i was attempting to push the merge commit here in an attempt to test it before submitting it to master

@tkelman
Copy link
Contributor

tkelman commented Apr 17, 2016

should only be 2 commits here, ee7f643 and its parent 10e9ef5, right? just force push the later one of those back over this branch?

this simplifies some of the code, and gives sharper type information.
fixes #5090, type inference of `&&` and `||` chains
also makes it easier to deal with #5560 and #14324
@vtjnash vtjnash merged commit c3e8bb7 into master Apr 19, 2016
@vtjnash vtjnash deleted the jb/constantprop branch April 19, 2016 03:34
@Keno
Copy link
Member

Keno commented May 2, 2016

@JeffBezanson: Any chance you could make the following infer Val{4} as the argument type to bar?

function foo(x)
    if x == 4
        bar(Val{x}())
    end
end

@JeffBezanson JeffBezanson mentioned this pull request Jul 15, 2016
65 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants