Skip to content

Explicit the order of evaluation for the select operation.#469

Closed
jcbeyler wants to merge 1 commit intoWebAssembly:masterfrom
jcbeyler:select
Closed

Explicit the order of evaluation for the select operation.#469
jcbeyler wants to merge 1 commit intoWebAssembly:masterfrom
jcbeyler:select

Conversation

@jcbeyler
Copy link

No description provided.

AstSemantics.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think the order is already specified, where it says All nodes other than control flow constructs need to evaluate their child nodes in the order they appear in the serialized AST. I feel that specifying it here redundantly might be confusing (people might think, why is this specified here, and why don't other ops say their order of evaluation).

Perhaps remove the first added sentence, and keep the last, replacing "therefore" with "note that"?

Copy link
Author

Choose a reason for hiding this comment

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

Done in new version

@kripken
Copy link
Member

kripken commented Nov 20, 2015

Thanks! But wait, re-reading that now, I'm confused - isn't the condition evaluated first, then the two values? (Maybe I have that wrong, and if so, my interpreter is wrong too ;)

Copy link
Member

Choose a reason for hiding this comment

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

no need for this comma

@jcbeyler
Copy link
Author

Good point, I don't know. It just seemed logical to me to do it this way :) But maybe someone has an opinion here?

@AndrewScheidecker
Copy link

It seems like translation to ASM.JS requires fewer temporaries if the condition is evaluated after the other two subexpressions. e.g. (select a.. b.. c..) ->

var _a, _b, _c;
_a = a..;
_b = b..;
_c = c..;
_a ? _b : _c;

vs

var _a, _b;
_a = a..;
_b = b..;
c.. ? _a : _b;

It seems like it also makes it easier to generate machine code that feed condition flags directly from the condition into cmov.

However, if we change the order it would be nice to change the parameter order so it matches the general left-to-right rule.

@jcbeyler
Copy link
Author

So do we want the first sentence to say:

select: a ternary operator with two operands, which must have the same type as each other, and a a boolean (i32) condition.

@rossberg
Copy link
Member

The first section in this doc explicitly specifies left-to-right evaluation order. That sometimes requires additional temporaries, but that's not specific to select.

@titzer
Copy link

titzer commented Nov 23, 2015

I'm OK changing the evaluation order of select by defining the condition to
come last. It doesn't make a different to TurboFan if there are no side
effects; it will generate exactly the same code.

On Mon, Nov 23, 2015 at 12:26 PM, rossberg-chromium <
notifications@github.com> wrote:

The first section in this doc explicitly specifies left-to-right
evaluation order. That sometimes requires additional temporaries, but
that's not specific to select.


Reply to this email directly or view it on GitHub
#469 (comment).

@sunfishcode
Copy link
Member

@AndrewScheidecker's idea about putting the condition last to better set up condition codes makes sense to me at first glance. I also agree that if we do this, we should change the operand order to that we don't have to make select an exception to the left-to-right rule.

@jfbastien
Copy link
Member

Agreed with what @sunfishcode just said.

@sunfishcode
Copy link
Member

I created #489 to propose reordering select's operands.

@sunfishcode
Copy link
Member

This is superseded by #489.

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.

7 participants