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

wasm non-trapping float-to-int conversions #5014

Merged
merged 4 commits into from Apr 26, 2018

Conversation

MikeHolman
Copy link
Contributor

@MikeHolman MikeHolman commented Apr 19, 2018

Implements the wasm proposal, as specified here:
https://github.com/WebAssembly/nontrapping-float-to-int-conversions

Closes #3228

@MikeHolman MikeHolman requested a review from Cellule April 19, 2018 02:05
@MikeHolman
Copy link
Contributor Author

Note: Tested locally, but there are no tests attached to the PR yet. I will push tests to the PR later once I figure out how to port the spec tests.

@Cellule
Copy link
Contributor

Cellule commented Apr 19, 2018

For the tests, I think you can look at what I did for Sign extension.
I added a feature folder in the WasmSpec test folder for that purpose.
#Closed

{
AnalysisAssert(!Saturate || doneLabel);
Copy link
Contributor

@Cellule Cellule Apr 20, 2018

Choose a reason for hiding this comment

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

I feel we should assert here that dst is int32 or uint32 #Closed

LoadFloatZero(zeroReg, instr);

m_lowerer->InsertCompareBranch(src64, zeroReg, Js::OpCode::BrGt_A, tooBigLabel, instr, true /*no NaN check*/);
instr->InsertBefore(IR::BranchInstr::New(Js::OpCode::JP, nanLabel, m_func));
Copy link
Contributor

@Cellule Cellule Apr 20, 2018

Choose a reason for hiding this comment

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

if dst is unsigned, we could just jump to nanLabel since both branch just set dst to 0 #Closed

@@ -314,6 +314,15 @@ MACRO_EXTEND_WMS( Conv_Check_FTUL , Long1Float1 , None
MACRO_EXTEND_WMS( Conv_Check_DTL , Long1Double1 , None )
MACRO_EXTEND_WMS( Conv_Check_DTUL , Long1Double1 , None )

MACRO_EXTEND_WMS( Conv_Sat_DTI , Int1Double1 , None )
Copy link
Contributor

@Cellule Cellule Apr 20, 2018

Choose a reason for hiding this comment

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

This will require an update of the ByteCode Guid (you'll see unit test failure in ChakraFull) #Closed

@@ -1260,6 +1260,13 @@
<tags>exclude_dynapogo</tags>
</default>
</test>
<test>
Copy link
Contributor

@Cellule Cellule Apr 20, 2018

Choose a reason for hiding this comment

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

In order to get this and not lose it next time I update the spec repo.
You have to add an entry in test\WasmSpec\convert-test-suite\config.json "folders": [ ... "features/nontrapping" ]
Then you need to run node convert-test-suite to generate rlexe.xml #Closed

instr->InsertBefore(oobLabel);
if (Saturate)
{
IR::LabelInstr * tooBigLabel = IR::LabelInstr::New(Js::OpCode::Label, m_func, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can eliminate the additional cmp with zero by doing the following

xorps dst, dst ;; mov dst, 0
cmp minInt, src1
jae truncMinInt
jp done ;; src1 is NaN and we've initialized dst to 0, so we're done
cmp maxInt, src1
ja conversion ;; src1 is in the valid range do the conversion
mov dst, maxInt ;; we know we're greater then maxInt
jmp done
:truncMinInt
mov dst, minInt
jmp done
:conversion
...
:done

We can either initialize dst to 0 and do jp done or do jp nan; ... :nan mov dst, 0; jmp done;.

Copy link
Contributor

@Cellule Cellule left a comment

Choose a reason for hiding this comment

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

:shipit:

@chakrabot chakrabot merged commit 7487cda into chakra-core:master Apr 26, 2018
chakrabot pushed a commit that referenced this pull request Apr 26, 2018
Merge pull request #5014 from MikeHolman:nontrapping

Implements the wasm proposal, as specified here:
https://github.com/WebAssembly/nontrapping-float-to-int-conversions

Closes #3228
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.

None yet

3 participants