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

Also perform uninitialized check on temp locals. #4252

Merged
merged 2 commits into from Oct 31, 2016

Conversation

Projects
None yet
3 participants
@headius
Member

headius commented Oct 26, 2016

Fixes #4251.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 26, 2016

Member

@subbuss I am confused though...I do not see any TemporaryLocalVariable handling before or after your patch, but obviously the issue in #4251 was exactly why we started initializing all temp locals eagerly to nil. What am I missing?

Member

headius commented Oct 26, 2016

@subbuss I am confused though...I do not see any TemporaryLocalVariable handling before or after your patch, but obviously the issue in #4251 was exactly why we started initializing all temp locals eagerly to nil. What am I missing?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 26, 2016

Member

@subbuss @headius I think our issue in this specific case is the optarg is missing an init when it is an lvar and then we convert it to a temp. So this fixes the problem but I think the underlying issue is we convert non-nil-corrected lvars into temp vars.

Another argument worth asking is should we worry about keeping this assumption that temp vars are always initialized or should we just add this extra logic and cover our asses?

Member

enebo commented Oct 26, 2016

@subbuss @headius I think our issue in this specific case is the optarg is missing an init when it is an lvar and then we convert it to a temp. So this fixes the problem but I think the underlying issue is we convert non-nil-corrected lvars into temp vars.

Another argument worth asking is should we worry about keeping this assumption that temp vars are always initialized or should we just add this extra logic and cover our asses?

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Oct 27, 2016

Contributor

This patch is incomplete. I need to page in this code into my memory. But, looks like I started doing the initialization for all vars (see buildDataflowVars which gets all vars, not just LocalVariables), and at some point decided to test for inits for only LocalVariables.

So, anywhere there is a check for LocalVariable in the code needs an update to not assume lvar. I can follow up if you want, or I can point you to other places to fix.

applyTransferFunction is the only other place it seems .. since tmps don't cross scope boundaries, identifyUndefinedVarsInClosure is correct to look at lvars.

Contributor

subbuss commented Oct 27, 2016

This patch is incomplete. I need to page in this code into my memory. But, looks like I started doing the initialization for all vars (see buildDataflowVars which gets all vars, not just LocalVariables), and at some point decided to test for inits for only LocalVariables.

So, anywhere there is a check for LocalVariable in the code needs an update to not assume lvar. I can follow up if you want, or I can point you to other places to fix.

applyTransferFunction is the only other place it seems .. since tmps don't cross scope boundaries, identifyUndefinedVarsInClosure is correct to look at lvars.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Oct 27, 2016

Contributor

You can reproduce the error with this snippet as well:

def foo(a)
  if (a)
    b = 1
  end
  p b
end

foo 0

You will find that foo will fail to JIT with the same error. This is because we run the the AddMissingInitsPass at the end to catch all possible scenarios of missing inits (which is why the code should work for tmps and lvars). In this case, the OptimizeDynamicVarsPass converts all the lvars to tmps and the tmps will have a missing init instead of the lvar. So, your patch is on the right track. You just need to fix applyTransferFunction as well.

Contributor

subbuss commented Oct 27, 2016

You can reproduce the error with this snippet as well:

def foo(a)
  if (a)
    b = 1
  end
  p b
end

foo 0

You will find that foo will fail to JIT with the same error. This is because we run the the AddMissingInitsPass at the end to catch all possible scenarios of missing inits (which is why the code should work for tmps and lvars). In this case, the OptimizeDynamicVarsPass converts all the lvars to tmps and the tmps will have a missing init instead of the lvar. So, your patch is on the right track. You just need to fix applyTransferFunction as well.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 27, 2016

Member

@subbuss I am agreeing it is better to have this work on all variable types now since we have hit this problem more than once; but if we move missing inits higher up the pass chain your snippet will not be broken anymore.

As a point of general guidance should all passes be made to work in any order as a principle do you think? Or should we just get better about enforcing dependencies?

Member

enebo commented Oct 27, 2016

@subbuss I am agreeing it is better to have this work on all variable types now since we have hit this problem more than once; but if we move missing inits higher up the pass chain your snippet will not be broken anymore.

As a point of general guidance should all passes be made to work in any order as a principle do you think? Or should we just get better about enforcing dependencies?

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Oct 27, 2016

Contributor

Both.

  1. It is more robust to have a pass work anywhere in the chain, and that is how some should work. I really meant to have AddMissingInitsPass work anywhere ... it is just plain oversight since I have been context switching jruby work across long periods of time and I lose attention. So, some of that can be improved by adding tests and specs.
  2. Some passes really can work after the IR is in some pre-determined state (ex: CFG has been built, or call protocol instrs haven't been added, code is in SSA form, etc.). So, there are also dependencies that have to be respected and enforced. In most cases, failed dependencies will fail fast. But, better to enforce them.
Contributor

subbuss commented Oct 27, 2016

Both.

  1. It is more robust to have a pass work anywhere in the chain, and that is how some should work. I really meant to have AddMissingInitsPass work anywhere ... it is just plain oversight since I have been context switching jruby work across long periods of time and I lose attention. So, some of that can be improved by adding tests and specs.
  2. Some passes really can work after the IR is in some pre-determined state (ex: CFG has been built, or call protocol instrs haven't been added, code is in SSA form, etc.). So, there are also dependencies that have to be respected and enforced. In most cases, failed dependencies will fail fast. But, better to enforce them.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 29, 2016

Member

So, your patch is on the right track. You just need to fix applyTransferFunction as well.

Great! I'll work on that once we're back from Austin.

Member

headius commented Oct 29, 2016

So, your patch is on the right track. You just need to fix applyTransferFunction as well.

Great! I'll work on that once we're back from Austin.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 31, 2016

Member

I added the fix for applyTransferFunction and re-pushed. I'll rebase as well so we can see it with recent greening on master.

Member

headius commented Oct 31, 2016

I added the fix for applyTransferFunction and re-pushed. I'll rebase as well so we can see it with recent greening on master.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Oct 31, 2016

Contributor

You will hate me, but now that I look at the code again, I was wrong. tmp vars don't leak in from outer scopes. Only lvars do! So, applyTransferFunction was correct as is. So, your previous fix was indeed complete. Sorry, but I seem to mix up this "simple" tmp / lvar distinction every once in a while.

Contributor

subbuss commented Oct 31, 2016

You will hate me, but now that I look at the code again, I was wrong. tmp vars don't leak in from outer scopes. Only lvars do! So, applyTransferFunction was correct as is. So, your previous fix was indeed complete. Sorry, but I seem to mix up this "simple" tmp / lvar distinction every once in a while.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 31, 2016

Member

@subbuss Oh, so like the closure var walker later in the file, this should only ever see LocalVariable. I'll remove my change and merge, since CI looks pretty good.

Member

headius commented Oct 31, 2016

@subbuss Oh, so like the closure var walker later in the file, this should only ever see LocalVariable. I'll remove my change and merge, since CI looks pretty good.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Oct 31, 2016

Contributor

Yes.

Can you please add JIT tests (either as part of the PR or in a followup commit)? The example I added earlier in #4252 (comment) should be good enough.

Contributor

subbuss commented Oct 31, 2016

Yes.

Can you please add JIT tests (either as part of the PR or in a followup commit)? The example I added earlier in #4252 (comment) should be good enough.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 31, 2016

Member

@subbuss This was already failing a couple JIT tests, but I'll add any cases that aren't covered.

Member

headius commented Oct 31, 2016

@subbuss This was already failing a couple JIT tests, but I'll add any cases that aren't covered.

@headius headius added this to the JRuby 9.1.6.0 milestone Oct 31, 2016

@headius headius merged commit b9b86fc into jruby:master Oct 31, 2016

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@headius headius deleted the headius:nil_init_for_temp_locals branch Oct 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment