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

Disable branchif/branchnil/branchunless optimization #3150

Closed

Conversation

jeremyevans
Copy link
Contributor

The comment for this block says:

This is super nasty hack!!!

In addition to apparently being a nasty hack, it's not
always correct, as it misoptimizes the following code
leading to a stack consistency error or crash:

def f;x = false; y = (return until x unless x);end;f

Someone with more experience with the optimizer and iseqs
should review the code and determine how it can be
changed to not break in the above code, and determine if
it is worth keeping the optimization. For now, the safe
solution is to just disable the optimization.

Fixes [Bug #16695]

@jeremyevans jeremyevans requested a review from mame May 27, 2020 18:05
The comment for this block says:

  This is super nasty hack!!!

In addition to apparently being a nasty hack, it's not
always correct, as it misoptimizes the following code
leading to a stack consistency error or crash:

  def f;x = false; y = (return until x unless x);end;f

Someone with more experience with the optimizer and iseqs
should review the code and determine how it can be
changed to not break in the above code, and determine if
it is worth keeping the optimization.  For now, the safe
solution is to just disable the optimization.

Fixes [Bug #16695]
@shyouhei
Copy link
Member

@ko1
Copy link
Contributor

ko1 commented May 28, 2020

This patch solves this issue (but introduce other issues):

@@ -6727,7 +6727,7 @@ compile_return(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node,
 	if (type == ISEQ_TYPE_METHOD) {
 	    splabel = NEW_LABEL(0);
 	    ADD_LABEL(ret, splabel);
-	    ADD_ADJUST(ret, line, 0);
+            // ADD_ADJUST(ret, line, 0);
 	}

The problem is ad-hoc stack calculation mechanism (ADJUST), and the issue is exposed because of stack calculation with peephole optimization.

@ko1
Copy link
Contributor

ko1 commented May 29, 2020

Nobu and I discussed this issue (how to solve), and we concluded we have three options:

(1) apply @jeremyevans 's patch to disable doubtful optimization
(2) apply ad-hoc solution
(3) re-design stack-depth fixing algorithm

For (2), we found there are successful code which are very similar to buggy code. We may find the difference and change the compiler to output successful code. This approach can solve this issue, but maybe there are other problematic cases.

https://hackmd.io/0c279ltHSUOxsVgmbFtZgA?both lists successful and problematic code.

The complete solution is (3). The essential problem is how to calculating the stack depth.

The code output similar to the following code:

  putnil # depth:1
  jump L1 # depth: 1  # (J1)
L2:
# stack depth is defined by the following (J2),
# but our simple compile.c code assume stack-depth is same as (J1)'s stack depth.
# To clear the stack-depth, pop is wrongly inserted.
  pop    # not needed!!
  putnil
  leave
L1:
  pop  # depth: 0
  jump L2  # (J2)

(3) is desirable, but it may consume the time because it is not easy coding (it requires carefully graph traversal programming). It is possible, no essentially difficult. But time consuming compare with priority.
Nobu will check the (2)'s possibility soon.

@jeremyevans
Copy link
Contributor Author

Closing this as @wanabe committed a better fix.

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

Successfully merging this pull request may close these issues.

3 participants