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

fix some cases where we incorrectly removed global var accesses #16106

Merged
merged 1 commit into from
May 7, 2016

Conversation

JeffBezanson
Copy link
Member

Possible fix for the issue identified here: #16090 (comment)

- if a global var might be undefined, consider it not `effect_free`
  since accessing it can throw an error.

- typeasserts that don't fire are removed by inlining, so don't
  consider typeassert `effect_free` since any remaining calls might
  throw.
@vtjnash
Copy link
Member

vtjnash commented Apr 29, 2016

iirc, this will cause performance regressions due to inhibiting the later optimization passes (such as tuple destructuring)

@JeffBezanson
Copy link
Member Author

Which part --- the global var part or the typeassert part? I wouldn't think accessing possibly-undefined global variables was that important a case.

@vtjnash
Copy link
Member

vtjnash commented Apr 29, 2016

the typeassert. i don't know of any cases where the globals access could cause issues either

@JeffBezanson
Copy link
Member Author

My thinking was that since we remove typeasserts that won't throw, any that remain indeed have effects that need to be preserved. Am I missing a case?

@vtjnash
Copy link
Member

vtjnash commented Apr 29, 2016

That would make sense (I forgot we have a custom inliner for this). I would have to look back through blame to check, but I think it was the printf perf test that was most noticably affected by this

@JeffBezanson
Copy link
Member Author

I'll check that.

@yuyichao
Copy link
Contributor

Will this fix the original issue in #9765? (maybe not the isa one)

@JeffBezanson
Copy link
Member Author

Yes it fixes the original issue there, but not all the cases mentioned in the later comments.

So far I don't really see any regressions here. meteor_contest is slightly slower, but given #16128 it might be noise.

@vtjnash vtjnash merged commit 3f59c0d into master May 7, 2016
@tkelman tkelman deleted the jb/globalvarchecks branch May 7, 2016 18:19
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.

3 participants