Optimize tuple.extract of gets in BinaryInstWriter #5941

tlively · 2023-09-14T21:08:24Z

In general, the binary lowering of tuple.extract expects that all the tuple
values are on top of the stack, so it inserts drops and possibly uses a scratch
local to ensure only the extracted value is left. However, when the extracted
tuple expression is a local.get, local.tee, or global.get, it's much more
efficient to change the lowering of the get or tee to ensure that only the
extracted value is on the stack to begin with. Implement that optimization in
the binary writer.

tlively · 2023-09-14T21:08:36Z

Current dependencies on/for this PR:

main
- PR [NFC] Port the multivalue round tripping test to lit #5942
  - PR Optimize tuple.extract of gets in BinaryInstWriter #5941 👈

This comment was auto-generated by Graphite.

codecov · 2023-09-14T21:23:23Z

Codecov Report

Merging #5941 (20d855e) into main (6dde22c) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #5941      +/-   ##
==========================================
- Coverage   42.80%   42.79%   -0.01%     
==========================================
  Files         485      485              
  Lines       74943    74963      +20     
  Branches    11958    11965       +7     
==========================================
+ Hits        32076    32084       +8     
- Misses      39665    39670       +5     
- Partials     3202     3209       +7

Files Changed	Coverage Δ
src/wasm-stack.h	`85.58% <ø> (+0.26%)`	⬆️
src/wasm/wasm-stack.cpp	`38.70% <50.00%> (+0.09%)`	⬆️

... and 4 files with indirect coverage changes

kripken

Nice!

kripken · 2023-09-14T21:22:39Z

test/multivalue.wast.fromBinary.noDebugInfo

-     )
-    )
-    (local.get $13)
-   )


Great improvement!

kripken · 2023-09-14T21:23:48Z

src/wasm-stack.h

@@ -148,6 +148,11 @@ class BinaryInstWriter : public OverriddenVisitor<BinaryInstWriter> {
  InsertOrderedMap<Type, Index> scratchLocals;
  void countScratchLocals();
  void setScratchLocals();
+
+  // local.get and local.tee expressions that will be followed by
+  // tuple.extracts. We can optimize these by only getting only the local


Suggested change

// tuple.extracts. We can optimize these by only getting only the local

// tuple.extracts. We can optimize these by getting only the local

kripken · 2023-09-14T21:28:42Z

src/wasm/wasm-stack.cpp

+    // We only need to get the single extracted value.
+    if (it->second == 0) {
+      o << int8_t(BinaryConsts::LocalTee)
+        << U32LEB(mappedLocals[std::make_pair(curr->index, it->second)]);


Suggested change

<< U32LEB(mappedLocals[std::make_pair(curr->index, it->second)]);

<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);

kripken · 2023-09-14T21:30:06Z

src/wasm/wasm-stack.cpp

+    o << int8_t(BinaryConsts::GlobalGet) << U32LEB(index + it->second);
+    return;
+  }
+  // Emit a global.get for each element if this is a tuple global


Wait, we allow tuple globals? Does the wasm spec?

Yes, we weren't originally going to support tuple globals, but when we first implemented all this tuple business we found that some passes needed them. I don't remember which ones those were, though.

The Wasm spec supports tuple globals only as much as it supports tuple locals, i.e. not at all. But that's ok!

Hmm... a little odd, but ok I guess 😄 I suppose the passes would need to create more globals or such, otherwise.

kripken · 2023-09-14T21:31:16Z

src/wasm/wasm-stack.cpp

@@ -2532,6 +2564,15 @@ void BinaryInstWriter::countScratchLocals() {
  for (auto& [type, _] : scratchLocals) {
    noteLocalType(type);
  }
+  // While we have all the tuple.extracts, also find extracts of local.gets and
+  // local.tees that we can optimize.


Comment could mention global gets too.

kripken · 2023-09-14T21:32:27Z

test/multivalue.wast

-  (global.get $g2)
+  (tuple.extract 1
+   (global.get $g2)
+  )


Should we also have a test of a global.get of a tuple that does not flow into an extract?

tlively · 2023-09-14T21:53:23Z

Rebased on top of a separate PR moving the tests to lit, which also made sure we are testing both an extract and non-extract use of a tuple global.get. The other comments have been addressed in the latest commit.

kripken · 2023-09-14T22:22:23Z

src/wasm/wasm-stack.cpp

+    o << int8_t(BinaryConsts::GlobalGet) << U32LEB(index + it->second);
+    return;
+  }
+  // Emit a global.get for each element if this is a tuple global


Hmm... a little odd, but ok I guess 😄 I suppose the passes would need to create more globals or such, otherwise.

In general, the binary lowering of tuple.extract expects that all the tuple values are on top of the stack, so it inserts drops and possibly uses a scratch local to ensure only the extracted value is left. However, when the extracted tuple expression is a local.get, local.tee, or global.get, it's much more efficient to change the lowering of the get or tee to ensure that only the extracted value is on the stack to begin with. Implement that optimization in the binary writer.

kripken · 2023-09-15T00:18:22Z

Fuzz bug:

(module
 (type $0 (func (result i64 f32 i32 i32 i32)))
 (type $1 (func (result f32)))
 (func $0 (type $0) (result i64 f32 i32 i32 i32)
  (local $3 (i64 f32 i32 i32 i32))
  (local $4 f32)
  (local.set $3
   (tuple.make
    (i64.const 0)
    (call $1)
    (i32.const 0)
    (i32.const 0)
    (i32.const 0)
   )
  )
  (local.set $4
   (tuple.extract 1
    (local.get $3)
   )
  )
  (tuple.make
   (i64.const 0)
   (f32.const 0)
   (i32.const 0)
   (i32.const 0)
   (i32.const 0)
  )
 )
 (func $1 (type $1) (result f32)
  (f32.convert_i32_s
   (i32.const 0)
  )
 )
)

STR:

$ bin/wasm-opt w.wat  --gufa-cast-all  --generate-stack-ir --optimize-stack-ir --roundtrip --shrink-level=1 -all
[wasm-validator error in function 0] local.set's value type must be correct, on 
(local.set $2
 (i32.const 0)
)
Fatal: error after opts

tlively · 2023-09-15T01:05:26Z

Oof, I'm not sure how we should handle locals that have been optimized out in the stack IR, actually. Do you have an idea?

This reverts commit 56ce1ea.

…5945) This reverts commit 56ce1ea. The binary writer optimization is not always correct when stack IR optimizations have run. Revert the change until we can fix it.

kripken · 2023-09-18T16:49:25Z

I think it might be tricky to combine the two. Really the tuples should be lowered before Stack IR (is in Poppy IR), to make this clean. But for now, how about disabling local2stack on tuple locals in StackIR? The benefit of that optimization is around 1-2% at best, while your benefit here is quite large.

In general, the binary lowering of tuple.extract expects that all the tuple values are on top of the stack, so it inserts drops and possibly uses a scratch local to ensure only the extracted value is left. However, when the extracted tuple expression is a local.get, local.tee, or global.get, it's much more efficient to change the lowering of the get or tee to ensure that only the extracted value is on the stack to begin with. Implement that optimization in the binary writer.

…ly#5941)" (WebAssembly#5945) This reverts commit 56ce1ea. The binary writer optimization is not always correct when stack IR optimizations have run. Revert the change until we can fix it.

tlively requested a review from kripken September 14, 2023 21:08

kripken reviewed Sep 14, 2023

View reviewed changes

tlively changed the base branch from main to move-mv-tests September 14, 2023 21:52

tlively force-pushed the better-tuple-extract branch from 952099a to 386b912 Compare September 14, 2023 21:52

tlively requested a review from kripken September 14, 2023 21:52

tlively mentioned this pull request Sep 14, 2023

[NFC] Port the multivalue round tripping test to lit #5942

Merged

tlively mentioned this pull request Sep 14, 2023

Tuples inhibiting optimizations #5923

Closed

kripken approved these changes Sep 14, 2023

View reviewed changes

Base automatically changed from move-mv-tests to main September 14, 2023 22:34

tlively added 2 commits September 14, 2023 15:36

address comments

20d855e

tlively force-pushed the better-tuple-extract branch from 386b912 to 20d855e Compare September 14, 2023 22:36

tlively enabled auto-merge (squash) September 14, 2023 22:37

tlively merged commit 56ce1ea into main Sep 14, 2023

tlively deleted the better-tuple-extract branch September 14, 2023 23:15

tlively added a commit that referenced this pull request Sep 15, 2023

Revert "Optimize tuple.extract of gets in BinaryInstWriter (#5941)"

227010a

This reverts commit 56ce1ea.

tlively mentioned this pull request Sep 15, 2023

Revert "Optimize tuple.extract of gets in BinaryInstWriter" #5945

Merged

	// tuple.extracts. We can optimize these by only getting only the local
	// tuple.extracts. We can optimize these by getting only the local

	<< U32LEB(mappedLocals[std::make_pair(curr->index, it->second)]);
	<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);

Optimize tuple.extract of gets in BinaryInstWriter #5941

Optimize tuple.extract of gets in BinaryInstWriter #5941

Uh oh!

Conversation

tlively commented Sep 14, 2023

Uh oh!

tlively commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Sep 15, 2023

Uh oh!

kripken commented Sep 18, 2023

Uh oh!

Uh oh!

tlively commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

kripken commented Sep 15, 2023 •

edited

Loading