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

Stateful: fix major bug in implementation #27972

Merged
merged 3 commits into from
Jul 10, 2018
Merged

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jul 6, 2018

And re-tighten Inference information for Const boolean converted to Conditional

fix #26417
fix #26339

@ararslan ararslan added the kind:bugfix This change fixes an existing bug label Jul 6, 2018
@ararslan ararslan requested review from jrevels and Keno July 6, 2018 20:33
@StefanKarpinski
Copy link
Sponsor Member

Was this a behavioral bug or a performance bug?

@Keno
Copy link
Member

Keno commented Jul 6, 2018

Implementation looks fine to me, but it's a bit odd that the commit message says this fixes a bug in Stateful, when neither of the bugs it fixes uses Stateful.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jul 6, 2018

Behavioral bug. Stateful was depending on Inference failing here in order to not reach its bad code (its tfunc model for iterate was wrong).

Missed optimizations introduced when the type lattice was changed by:

    commit 146c2ba
    Author: Keno Fischer <keno@alumni.harvard.edu>
    Date:   Tue Jan 16 13:56:21 2018 -0500

        Fix backpropagation of conditionals when the type is later widened
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jul 10, 2018

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

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

@vtjnash vtjnash merged commit 1d55130 into master Jul 10, 2018
@vtjnash vtjnash deleted the jn/Stateful-26339-26417 branch July 10, 2018 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inference regression with === nothing Suboptimal inference of === with Union{}
5 participants