Skip to content

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

Merged
chakrabot merged 4 commits into
chakra-core:masterfrom
MikeHolman:nontrapping
Apr 26, 2018
Merged

wasm non-trapping float-to-int conversions#5014
chakrabot merged 4 commits into
chakra-core:masterfrom
MikeHolman:nontrapping

Conversation

@MikeHolman

@MikeHolman MikeHolman commented Apr 19, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Cellule commented Apr 19, 2018

Copy link
Copy Markdown
Contributor

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

IR::Opnd*
LowererMD::GenerateTruncChecks(_In_ IR::Instr* instr, _In_opt_ IR::LabelInstr* doneLabel)
{
AnalysisAssert(!Saturate || doneLabel);

@Cellule Cellule Apr 20, 2018

Copy link
Copy Markdown
Contributor

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

Comment thread lib/Backend/LowerMDShared.cpp Outdated
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));

@Cellule Cellule Apr 20, 2018

Copy link
Copy Markdown
Contributor

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

MACRO_EXTEND_WMS( Conv_Check_DTL , Long1Double1 , None )
MACRO_EXTEND_WMS( Conv_Check_DTUL , Long1Double1 , None )

MACRO_EXTEND_WMS( Conv_Sat_DTI , Int1Double1 , None )

@Cellule Cellule Apr 20, 2018

Copy link
Copy Markdown
Contributor

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

Comment thread test/WasmSpec/rlexe.xml
<tags>exclude_dynapogo</tags>
</default>
</test>
<test>

@Cellule Cellule Apr 20, 2018

Copy link
Copy Markdown
Contributor

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

Comment thread lib/Backend/LowerMDShared.cpp Outdated
instr->InsertBefore(oobLabel);
if (Saturate)
{
IR::LabelInstr * tooBigLabel = IR::LabelInstr::New(Js::OpCode::Label, m_func, true);

Copy link
Copy Markdown
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;.

@Cellule Cellule left a comment

Copy link
Copy Markdown
Contributor

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.

3 participants