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

interpreter: fix bug with ssair phi-node handling #29262

Merged
merged 4 commits into from
Oct 1, 2018
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Sep 18, 2018

@JeffBezanson JeffBezanson added the needs tests Unit tests are required for this change label Sep 19, 2018
@vtjnash
Copy link
Member Author

vtjnash commented Sep 19, 2018

@Keno This is failing the 32-bit build. Can you tell me how to fix that?

@vtjnash
Copy link
Member Author

vtjnash commented Sep 26, 2018

For adding tests, I plan to run the core.jl tests with the interpreter (takes a couple minutes). There's just one test that needs to be fixed.

@JeffBezanson
Copy link
Member

I would rather have an example of a specific function that fails. If an existing test in core.jl is sufficient that's fine, we should just copy it into a new test that runs it with --compile=min.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 26, 2018

I know the approximate forms of IR that would fail, but I think it's also useful to show that our interpreter is an accurate representation of our compiler.

@JeffBezanson
Copy link
Member

Yes, I agree it's useful to run some subset of the tests both in the interpreter and compiler, but we also need an example that's known to trigger this problem. Otherwise e.g. moving tests out of core.jl or other test changes could inadvertently stop testing for this.

somehow these were missed in the last round of `nospecialize` annotations in this file,
but these are also very important for compiler performance
(especially as we have now been adding more places where we inspect tuples with this method)
it is not valid to treat these like statements, since they are actually arguments to the basic block
it turns out to be a bit hard to deal with this particular representation of phi nodes in an interpreter,
since the information is spread out across multiple statements and could pop up at any time,
but with some care, it is possible. we just need to be careful to process phi nodes as a block,
and taking care to look for implicit edges.

fix #29342
@vtjnash vtjnash merged commit 61696f9 into master Oct 1, 2018
@vtjnash vtjnash deleted the jn/interpret-phi branch October 1, 2018 15:21
@vtjnash vtjnash removed the needs tests Unit tests are required for this change label Oct 1, 2018
@@ -38,7 +38,10 @@ _setindex(v, i::Integer) = ()

## iterating ##

iterate(t::Tuple, i::Int=1) = 1 <= i <= length(t) ? (@inbounds t[i], i+1) : nothing
function iterate(@nospecialize(t::Tuple), i::Int=1)
Copy link
Member

Choose a reason for hiding this comment

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

So #29147 is fixed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs nanosoldier run This PR should have benchmarks run on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants