Skip to content

Factor a multiblock node out of tableswitch. #490

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

Closed
wants to merge 1 commit into from
Closed

Conversation

sunfishcode
Copy link
Member

As discussed in #480, this introduces multiblock, and simplifies tableswitch accordingly.

This also updates the wording around control flow operators in several areas.

* `tableswitch`: a jump table which may jump either to an immediate `case`
child or to a label in an enclosing construct
* `case`: a case which must be an immediate child of `tableswitch`
* `br`: branch to a given label
Copy link
Member

Choose a reason for hiding this comment

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

removing "in an enclosing construct" makes this less clear imo

Copy link
Member

Choose a reason for hiding this comment

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

perhaps that comment wasn't clear enough as well ;) I mean, why not write "in a visible construct" using the new terminology?


Branches may only reference *visible* labels. The labels in `block` and `loop`
nodes are visible only from within their subtrees. The labels in `multiblock`
nodes are only visible from within their subtrees, and only from above their
Copy link
Member

Choose a reason for hiding this comment

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

"above" has the same issues as "left"/"right". How about "before"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Updated.

@kripken
Copy link
Member

kripken commented Dec 3, 2015

Would we actually still need block, if we had multiblock?

@sunfishcode
Copy link
Member Author

I imagine block and multiblock might be encoded differently, but otherwise no, multiblock could subsume block.

@kripken
Copy link
Member

kripken commented Dec 4, 2015

Some more thoughts: overall I like this, but also feel like we should gather some data first. In particular, this construct can subsume block, if and if_else, so just adding it leads to some redundancy - which is perhaps justified by efficiency, but we need data to know.

@lukewagner
Copy link
Member

Since multiblock would not be the redundant op, could we consider the issue of whether we still need the other ops as a separate issue?

@ghost
Copy link

ghost commented Dec 4, 2015

On 12/05/2015 10:06 AM, Alon Zakai wrote:

Some more thoughts: overall I like this, but also feel like we should
gather some data first. In particular, this construct can subsume block,
if and if_else, so just adding it leads to some redundancy - which is
perhaps justified by efficiency, but we need data to know.

I think it would be unfortunate to remove if and if_else and require them to be expressed only as multiblock and br_if and br_unless from the point of view of giving some consideration to the source code being readable and writeable. Also keep in mind that br_if always evaluates the result expression, and we still need if when wanting to conditionally evaluate the result.

@kripken
Copy link
Member

kripken commented Dec 5, 2015

@lukewagner I personally would prefer to consider the issue as a whole, as opposed to adding all the possibilities and weeding out the unnecessary ones later. I say "all" because there are other control flow proposals as well, like br_unless and unless, perhaps other options for switch, etc. ,which are also relevant here.

And given that we should be able to get relevant data quite soon, focusing on that seems to make more sense to me.

But, I don't feel strongly about this.

@JSStats: I believe you can still conditionally evaluate a result using multiblock, you just br forward to jump over the computation, so it becomes conditional.

@ghost
Copy link

ghost commented Dec 5, 2015

@kripken multiblock meets use cases expected to be common in some code, but not in other code. Hand written code might be much more structured, and make sparing use of multiblock, but it will be interesting to see where it can be used with clarity. Thus I support @lukewagner's call to move the discussion about other operators to new issues.

It's probably still computational complete, but how readable and writeable??

x = 2 * a + { br_if(, fn1(a) < 10, $next); br(fn2(a, b), $done); $next; fn3(a, b); $done; } + k;
versus
x = 2 * x + if_else((fn1(a) < 10), fn3(a, b), fn2(a, b)) + k;

I would be surprised if this did not show up in encoding efficiency too.

This also updates the wording around control flow operators in
several areas.
@sunfishcode
Copy link
Member Author

A lot of the discussion here has focused on the multiblock construct. As a possible easier way forward, I've submitted #512 which is a subset of this PR that just removes case without introducing multiblock.

@ghost
Copy link

ghost commented Jan 4, 2016

Perhaps introducing the multiblock before removing case would be an easier path.

@sunfishcode
Copy link
Member Author

There is not consensus to add multiblock at this time.

@jfbastien jfbastien deleted the multiblock branch February 26, 2016 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants