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

Parallel value assignment broken in IR builder (multi-value literals, calls, attr-assign, etc.) #2574

Closed
jowl opened this Issue Feb 6, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@jowl
Copy link
Contributor

jowl commented Feb 6, 2015

It appears as if variables in expressions are evaluated at a later stage in jruby-head than in MRI and JRuby 1.7.19, as shown in the example below.

$ rvm jruby-head,jruby-1.7.19,2.2.0 do ruby -e 'a=0;b=[a,a=1];p(b)'
[1, 1]
[0, 1]
[0, 1]

Or more readable

a = 0
[a, (a = 1)] # => [1, 1]

This is as minimal an example as I've been able to produce, and I don't really know where to continue the investigation.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 6, 2015

Interesting bug :-) .. Looks like our IR builder is not respecting ordering here .. Generated instrs:

        a(0:0) = copy(Fixnum:0)
        a(0:0) = copy(Fixnum:1)
        %v_3 = copy(Array:[a(0:0), Fixnum:1])

So, clearly broken.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 7, 2015

This was a "2 bugs for the price of 1" issue :-). See eb3740c

@subbuss subbuss closed this in cc00fd4 Feb 7, 2015

@jowl

This comment has been minimized.

Copy link
Contributor Author

jowl commented Feb 7, 2015

Good job in finding the problem and fixing it so fast. I've confirmed that your fix indeed solves the array construction issue, but it seems that the rabbit hole is even deeper; the same behavior is observed in hash construction as well, for both keys and values.

a = 0
{ a => a, (a = 1) => a } # => { 1 => 1 } (MRI: {0=>0, 1=>1})
{ a => a, a => (a = 2) } # => { 2 => 2 } (MRI: {1=>2})

This has been observed on cc00fd4.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 7, 2015

Yes, I was going to audit the other literals and fix them. But, good to get confirmation that others might be broken similarly.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 7, 2015

The fix is fairly trivial .. just have to systematically go through all literals and fix the ones that need fixing.

subbuss added a commit that referenced this issue Feb 8, 2015

Fix #2574: Part 3: Fix IR building of hash literals
* Hash literals were broken similarly as array literals.
* Updated the spec for #2574.
@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 8, 2015

I don't see any other multi-valued literal besides arrays and hashes. If I missed any, please update this issue.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 8, 2015

Will check call args and masgns next.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Feb 8, 2015

You unearthed a gold mine of a bug here! :) A lot of parallel value construction in many call scenarios are broken. Interesting that this kind of bug never surfaced in mri tests or rubyspec.

a = 10
b = a / (a=10)
p b # prints 1

OR

a = "xyz"
p (a =~ (a = /[x]/)); # throws an exception

@enebo @headius fyi .. so, the real brain dead fix would be to just force all build values to go through 'copyAndReturnValue' and let OptimizeTempVarsPass, LocalOptPass, etc. propagate useless copies (which there will be lots of now). Otherwise, we'll have to look at all parallel value construction in calls, attr assign, etc. and fix up where required (even so, there will be lots of copies).

For the array and hash literal construction cases, after those passes and DCE runs, the output is:

        %t_b_4 = copy({Fixnum:0=>Fixnum:0, Fixnum:1=>Fixnum:1})
        %t_c_5 = copy({Fixnum:1=>Fixnum:1, Fixnum:1=>Fixnum:2})
....
        %t_b_3 = copy(Array:[Fixnum:0, Fixnum:1])

So, those passes will take care of killing useless instrs. but will defer fixing this till we've had a chance to chat about this.

@subbuss subbuss reopened this Feb 8, 2015

@subbuss subbuss changed the title Too lazy evaluation in JRuby 9K Parallel value assignment broken in IR builder (multi-value literals, calls, attr-assign, etc.) Feb 8, 2015

@subbuss subbuss added this to the 9.0.0.0.pre2 milestone Feb 8, 2015

@subbuss subbuss added the ir label Feb 8, 2015

enebo added a commit that referenced this issue Feb 11, 2015

Make hasAssignment hasVariableAssignment since it only refers to lvar…
…/dvars.

Use this new knowledge in IRBuilder to fix GH #2574.  Still some other cases
to fix.

enebo added a commit that referenced this issue Feb 11, 2015

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Feb 12, 2015

I am resolving this. I covered the cases apparent where we have a list of arguments which must be processed. It is possible some cases were missed (regression spec should cover broad cases known), but if any new cases are found a new issue should be opened.

@enebo enebo closed this Feb 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.