Skip to content

Consider NaN falseish #933

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

Merged
merged 4 commits into from
Nov 5, 2019
Merged

Consider NaN falseish #933

merged 4 commits into from
Nov 5, 2019

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Nov 1, 2019

fixes #770
fixes #787

This is mostly identical to #787 but also removes some internal patterns prone to errors when dealing with temporary locals as it was hard to grasp if code would "just work" otherwise.

In particular, flows had a getAndFreeTempLocal helper that immediately free'd a local intended for one-time use, which can lead to subtle errors where sub-expressions might reuse the same local. Hence I decided that even if code looks more cumbersome without it, that it's worth to require holding on to a temp as long as another make* function might reuse the same, so it's pretty much guaranteed that even if we update codegen in between a getTempLocal and a freeTempLocal in the future the resulting code will keep working.

I've also looked over all the occurrences of makeIsFalseish and makeIsTrueish which all seem to be fine, adding the local to the flow being responsible. The notable ones here are those of anything if-like where the condition temp locals go to the outer instead of one of the inner flows.

cc @MaxGraey

@MaxGraey
Copy link
Member

MaxGraey commented Nov 1, 2019

LGTM

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 1, 2019

Last commit also eliminates makeIsFalseish which was used only once when compiling !x, that is essentially "not is trueish" anyway.

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 4, 2019

Last commit makes isNaN and isFinite builtins again. Leads to slightly more code but also guarantees that these precompute at some places, which might be beneficial in static branch elimination. cc @MaxGraey wdyt?

@MaxGraey
Copy link
Member

MaxGraey commented Nov 4, 2019

I think this problem need solve via enhancing inlining limits. Yes we could improve isNaN and isFinite but this not solve problem with user space which still required manual inlining. Also I wonder how this better than simply add @inline decorators?

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 4, 2019

It's slightly more efficient than @inlineing since it doesn't create a block with aliased locals, but functionally equivalent and should yield about the same optimized code. Both would precompute.

@dcodeIO dcodeIO merged commit 8759033 into master Nov 5, 2019
@dcodeIO dcodeIO deleted the issue-770 branch November 8, 2019 01:59
@MaxGraey
Copy link
Member

MaxGraey commented Aug 17, 2020

Found one missing part. When we have casting from float to boolean like:

function cast(x: f64): bool {
   return bool(x);
}

it becomes:

return x != 0;

without isNaN check. So perhaps better implicitly transform to !!x for floats. WDYT?

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 17, 2020

Hmm, yeah, that's odd. Not sure what you mean with !!x, though. Looks like this needs to be handled on codegen level?

@MaxGraey
Copy link
Member

Not sure what you mean with !!x

Oh, mean it just should generate same code (x != 0.0) & (x == x)

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 17, 2020

Oh no, I spot a temporary local there. But yeah, guess this can't be avoided :)

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.

Different with JS behaviour for NaN || FloatValue
3 participants