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

Update Binaryen and other dependencies #1224

Merged
merged 4 commits into from
Apr 15, 2020
Merged

Update Binaryen and other dependencies #1224

merged 4 commits into from
Apr 15, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Apr 14, 2020

As usual, this updates Binaryen and other dependencies to the latest and greatest.

Includes

  • v128.abs<T> for 8/16/32 bit integers incl. i8x16.abs, i16x8.abs, i32x4.abs
  • v128.bitmask<T> including i8x16.bitmask, i16x8.bitmask, i32x4.bitmask
  • tuple.create / tuple.extract pseudo instructions for multi value types
  • BinaryenType now being usize, unfortunately triggering lots of 32/64 conversion warnings in switches when compiling to Wasm

global.set $abi/condition
i32.const 0
global.set $abi/y
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting :)

Copy link
Member

@MaxGraey MaxGraey Apr 15, 2020

Choose a reason for hiding this comment

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

it's ok. I told you earlier that binaryen improved some passes)

Comment on lines -315 to -338
i32.lt_s
global.set $binary/b
global.get $binary/i
i32.const 1
i32.gt_s
global.set $binary/b
global.get $binary/i
i32.const 1
i32.le_s
global.set $binary/b
global.get $binary/i
i32.const 1
i32.ge_s
global.set $binary/b
global.get $binary/i
i32.const 1
i32.eq
global.set $binary/b
global.get $binary/i
i32.const 1
i32.eq
global.set $binary/b
global.get $binary/i
i32.const 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Even more interesting

Comment on lines -7 to -16
(global $constructor/emptyCtor (mut i32) (i32.const 0))
(global $constructor/emptyCtorWithFieldInit (mut i32) (i32.const 0))
(global $constructor/emptyCtorWithFieldNoInit (mut i32) (i32.const 0))
(global $constructor/none (mut i32) (i32.const 0))
(global $constructor/justFieldInit (mut i32) (i32.const 0))
(global $constructor/justFieldNoInit (mut i32) (i32.const 0))
(global $constructor/ctorReturns (mut i32) (i32.const 0))
(global $constructor/ctorConditionallyReturns (mut i32) (i32.const 0))
(global $constructor/ctorAllocates (mut i32) (i32.const 0))
(global $constructor/ctorConditionallyAllocates (mut i32) (i32.const 0))
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice

global.set $~argumentsLength
i32.const 2
global.set $~argumentsLength
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixture apparently evaporated entirely

i32.const 2
i32.const 3
i32.const 1
call $function-types/doAddWithFn<i32>
call_indirect (type $i32_i32_=>_i32)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unfortunate, perhaps a regression?

Copy link
Member

@MaxGraey MaxGraey Apr 15, 2020

Choose a reason for hiding this comment

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

Hmm, perhaps need some extra tune of passes pipeline. Will try it after this pr

Copy link
Member

@MaxGraey MaxGraey Apr 15, 2020

Choose a reason for hiding this comment

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

I get the point!
$function-types/doAddWithFn<i32> was:

(func $function-types/doAddWithFn<i32> (; 5 ;) (param $0 i32) (param $1 i32) (param $2 i32) (result i32)
  i32.const 2
  global.set $~argumentsLength
  local.get $0
  local.get $1
  local.get $2
  call_indirect (type $i32_i32_=>_i32)
 )

after current optimizations it became:

(func $function-types/doAddWithFn<i32> (; 5 ;) (param $0 i32) (param $1 i32) (param $2 i32) (result i32)
  call_indirect (type $i32_i32_=>_i32)
)

and inlined so everything alright!

PS Perhaps make sense add one extra directize pass for further optimization. Will investigate later.

Copy link
Contributor

@jtenner jtenner left a comment

Choose a reason for hiding this comment

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

I have so much to learn. Just had a question.

(func $~start (; 1 ;)
i32.const 0
global.set $~argumentsLength
(func $~setArgumentsLength (param $0 i32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a bug? I suppose if the binary never uses the global that's fine, but it seems like anyone inspecting the global would expect this function to modify it.

Just curious what this means.

Copy link
Member Author

@dcodeIO dcodeIO Apr 14, 2020

Choose a reason for hiding this comment

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

Looks like the global is never read, so Binaryen figured it can remove it entirely. If there'd be inspection code, it would be read (somewhere, somewhen), but otherwise it's a blackbox anyway and should be fine to remove.

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