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

Handle the unsigned shift operator in dart2js #30890

Closed
floitschG opened this issue Sep 26, 2017 · 8 comments
Closed

Handle the unsigned shift operator in dart2js #30890

floitschG opened this issue Sep 26, 2017 · 8 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js

Comments

@floitschG
Copy link
Contributor

This is the dart2js issue for #30886.

This is a non-breaking change and is not required to be in Dart 2.0.
Probably blocked by #30889.

@rakudrama
Copy link
Member

dart2js canonicalizes all bit operations and shifts to unsigned 32-bit values.
So this is the same as the current 'signed' >> operator.

We might in future make >> return a signed result. This is quite involved since >> is optimized, requiring all optimizations to be updated (including type analysis and js_runtime helpers for special cases).

@floitschG
Copy link
Contributor Author

The int operation should be easy, but we would also want to make the operator user-overridable.

@matanlurey
Copy link
Contributor

I'm fine with this operator not being overridable in dart2js FWIW.

@stephan-gruen
Copy link

stephan-gruen commented Oct 4, 2017

dart2js canonicalizes all bit operations and shifts to unsigned 32-bit values.

Edit: only in Dartium

But
print('-1 >> 0 = ${-1 >> 0}');

unminified JS result:
-1 >> 0 = -1

minified JS result:
-1 >> 0 = 4294967295

Dart2js minified vs non-minified treats shifts differently if value < 0

@lrhn
Copy link
Member

lrhn commented Apr 30, 2019

This operator is an overridable operator that is available in general.

The int.operator>>> method is a separate concern, but one which will need some thought in JS compiled code.
It will likely not be equivalent to int.>> which does sign-extending shift of Int32 values, then converts to Uint32. The int.>>> operator should probably do non-sign-extending shift of Int32 values, then convert to Uint32. That is where the Dart x >> y is implemented in JS as (x >> y) >>> 0, then x >>> y in Dart can be implemented as just x >>> y in JS.

@vsmenon vsmenon added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jul 20, 2019
@leafpetersen leafpetersen added this to the March Beta Release milestone Feb 9, 2021
@rakudrama
Copy link
Member

rakudrama commented Feb 11, 2021

Optimization tasks

  • Global type analysis should infer a narrowed type for the result of int.>>>
  • Add a dynamic specializer to lower to SSA (existing HShiftRight), or specialized entry point (e.g. shift non-negative but otherwise unknown)
  • Verify algebraic simplifications. Since there is an existing HShiftRight SSA instruction that corresponds to JavaScript >>>, the algebraic simplification rules that currently work HShiftRight from int.>> should work for int.>>> too, but there might be edge cases not covered due to assumptions HShiftRight was lowered from int.>>.

@rakudrama rakudrama self-assigned this Mar 5, 2021
@rakudrama
Copy link
Member

Implementation done (39aa454).
Once --enable-experiment=triple-shift is the default, there are some cleanup tasks:

  • remove triple-shift flags added to the codegen and optimization tests
  • run dartfmt on the test code containing >>>.

@sigmundch
Copy link
Member

I decided to split up the cleanup into a separate issue, so we can properly track the fact that the feature is ready to ship.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js
Projects
None yet
Development

No branches or pull requests

8 participants