Skip to content

fix(compiler): wrap RHS shifting value by maximum size of type in bits for small integers #1511

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 5 commits into from
Oct 20, 2020
Merged

Conversation

MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Oct 19, 2020

Similar to #1494 we should wrap RHS shifting value by maximum size of type in bits.

For booleans we just return leftExpr due to leftExpr <<>> rightExpr & (1 - 1) -> leftExpr <<>> 0 -> leftExpr

  • I've read the contributing guidelines

@dcodeIO
Copy link
Member

dcodeIO commented Oct 19, 2020

The comment there reads // Cares about garbage bits on the RHS, but only for types smaller than 5 bits (4 bits if i16, 3 bits if i8), making me wonder why we'd want to mask these in addition to relying on that? Might well be that I misunderstood something there.

@MaxGraey
Copy link
Member Author

We need this masks because otherwise it interpret as UB. WebAssembly always use masks for i32/i64 shifts. So we need do the same for emulated types as well. Regarding other langs. C++ don't do this but it's well-known and documented UB. Rust for example always do this (and for small integers as well)

@dcodeIO
Copy link
Member

dcodeIO commented Oct 19, 2020

Ah, I see, so this interpolates shift semantics as if there'd be actual i16.shl and i8.shl instructions with 4/3 relevant bits. Should probably document this, but otherwise looks good to do.

@MaxGraey
Copy link
Member Author

You probably better person to properly document this. So you're free to do in this PR or later)

@dcodeIO dcodeIO merged commit 9b57abd into AssemblyScript:master Oct 20, 2020
@MaxGraey MaxGraey deleted the fix-small-shifts branch October 20, 2020 14:26
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.

2 participants