Fix binary emitting of br_if with a refined value by emitting a cast by kripken · Pull Request #6510 · WebAssembly/binaryen

kripken · 2024-04-17T23:45:31Z

Alternative to #6390: That PR makes br_if with a value have the same typing rules
as the wasm spec, i.e., the potentially less refined type of the break target rather than
the value itself. This PR, instead, works around that difference: we keep using the more
refined type in Binaryen IR, and when we emit a binary we make sure to fix it up so it
validates.

The rationale for using a somewhat hackish approach like that is that we hope the wasm
spec will improve here, and so this is will only be temporary in that case. Even if not,
this is useful because by using the most refined type in the IR we optimize in the best
way possible, and only suffer when we emit fixups in the binary, but in practice those
cases are very rare: br_if is almost always dropped rather than used, in real-world
code (except for fuzz cases and exploits).

In more detail, the fixup we perform is to add a cast when we see that the br_if's value
is used (not dropped) and it is not refined enough. The cast forces it to be the same
type as the br_if's value, basically.

kripken · 2024-05-14T22:50:03Z

The preparation work in StackIR is done but meanwhile @tlively landed a nice refactoring of scratch locals, and I realized I can use that here 😄 Basically this PR was a bit complex as it added a specific new mechanism for scratch locals, while #6583 adds a general mechanism for multiple scratch locals of a particular type. As evidence of the nice design there, it was easy to use here.

So my inclination now is that this PR is the way to do. Separately, we can consider whether we want to refactor more binary writing logic into StackIR, but that is no longer needed or urgent.

Top comment has been updated.

tlively · 2024-05-15T02:18:58Z

+ auto localIndex = scratchLocals[t] + scratchTypeUses[t];
+ scratchTypeUses[t]++;


This avoids having to do the lookup twice.
Suggested change
auto localIndex = scratchLocals[t] + scratchTypeUses[t];
scratchTypeUses[t]++;
auto localIndex = scratchLocals[t] + scratchTypeUses[t]++;

I worry it's a little less readable that way... and I'd hope the optimizer avoids a double lookup? What do you think?

A quick test on godbolt shows that even in the simplest possible case, the optimizer is not able to figure out how to avoid two lookups, unfortunately. I'd go with the single lookup solution, but I agree it's probably not that important.

Interesting it can't be optimized... I wonder why. I'll modify this then.
Seems like there is no other aliasing operation that can interfere, but I guess I was overly optimistic...

tlively · 2024-05-15T02:19:50Z

+ scratchTypeUses[t]--;
+ auto localIndex = scratchLocals[t] + scratchTypeUses[t];


Suggested change
scratchTypeUses[t]--;
auto localIndex = scratchLocals[t] + scratchTypeUses[t];
auto localIndex = scratchLocals[t] + --scratchTypeUses[t];

tlively

Did we definitely decide to go with the cast approach rather than the tee/drop/get approach?

tlively · 2024-05-15T02:47:59Z

+ if (unrefinedType == curr->type) {
+ // It has the proper type anyhow.
+ return;
+ }


After #6579 there will be no need to know the source type when emitting a cast, so this check will be the only remaining use for unrefinedType. This is also the only check that cannot be done as-is in ScratchLocalFinder. WDYT about making ScratchLocalFinder an ExpressionStackWalker to avoid needing this separate walker at all?

The overhead is annoying, I'm afraid, and I'd really like to not regress the by-far common case of not needing the extra work.
Some data:
$ perf stat -r 5 bin/wasm-opt UE4Game-HTML5-Shipping.wasm -all -o a.wasm warning: no passes specified, not doing any work warning: no passes specified, not doing any work warning: no passes specified, not doing any work warning: no passes specified, not doing any work warning: no passes specified, not doing any work Performance counter stats for 'bin/wasm-opt UE4Game-HTML5-Shipping.wasm -all -o a.wasm' (5 runs): 10,947.27 msec task-clock # 1.767 CPUs utilized ( +- 0.38% ) 855 context-switches # 78.102 /sec ( +- 9.24% ) 40 cpu-migrations # 3.654 /sec ( +- 11.87% ) 239,470 page-faults # 21.875 K/sec ( +- 0.07% ) 34,826,556,461 cycles # 3.181 GHz ( +- 0.27% ) 48,135,304,544 instructions # 1.38 insn per cycle ( +- 0.01% ) 10,101,559,953 branches # 922.747 M/sec ( +- 0.02% ) 150,681,797 branch-misses # 1.49% of all branches ( +- 1.17% ) 6.1962 +- 0.0404 seconds time elapsed ( +- 0.65% )
And, with this patch:
$ git diff diff --git a/src/wasm/wasm-stack.cpp b/src/wasm/wasm-stack.cpp index 53a25d52b..1a83f56cb 100644 --- a/src/wasm/wasm-stack.cpp+++ b/src/wasm/wasm-stack.cpp@@ -2681,7 +2681,7 @@ void BinaryInstWriter::noteLocalType(Type type, Index count) { } InsertOrderedMap<Type, Index> BinaryInstWriter::countScratchLocals() { - struct ScratchLocalFinder : PostWalker<ScratchLocalFinder> {+ struct ScratchLocalFinder : ExpressionStackWalker<ScratchLocalFinder> { BinaryInstWriter& parent; InsertOrderedMap<Type, Index> scratches;
$ perf stat -r 5 bin/wasm-opt UE4Game-HTML5-Shipping.wasm -all -o b.wasm Performance counter stats for 'bin/wasm-opt UE4Game-HTML5-Shipping.wasm -all -o b.wasm' (5 runs): 11,311.23 msec task-clock # 1.727 CPUs utilized ( +- 0.26% ) 720 context-switches # 63.654 /sec ( +- 5.01% ) 29 cpu-migrations # 2.564 /sec ( +- 8.30% ) 238,817 page-faults # 21.113 K/sec ( +- 0.04% ) 36,257,517,592 cycles # 3.205 GHz ( +- 0.14% ) 50,906,632,590 instructions # 1.40 insn per cycle ( +- 0.66% ) 10,525,197,435 branches # 930.509 M/sec ( +- 0.01% ) 155,907,135 branch-misses # 1.48% of all branches ( +- 0.70% ) 6.5514 +- 0.0335 seconds time elapsed ( +- 0.51% )
That's about 5% more instructions and walltime. Note that that measures load+save, so the impact on save is around double that.

Oof, that's way too much slowdown :(

kripken · 2024-05-15T18:07:55Z

Did we definitely decide to go with the cast approach rather than the tee/drop/get approach?

tee/drop/get requires StackIR, I think - hard to do it otherwise. I do have a prototype of that working, but it requires a lot of other infrastructure changes to support it. So I think the cost of a cast here is not that bad - this is a pretty rare situation.

tlively · 2024-05-15T18:35:34Z

I don't think it should require StackIR since it's just another drop-in replacement instruction sequence that only requires scratch locals. We should have all the necessary infrastructure in place. Besides the cost of the cast, I'm concerned that sooner or later we're going to end up with types that cannot be cast because rossberg keeps insisting that this would be reasonable and desirable. Would it be helpful if I put together a PR using that approach that we could compare against?

kripken · 2024-05-15T18:40:34Z

Hmm, thanks, but let me try myself, should take just a few minutes.

I remember that one concern I had with doing it that way was that it could not integrate with the tuple.extract optimizations. But doing it in StackIR, which is earlier, allowed that. But maybe that's not important enough. Let me try it out here and see.

kripken · 2024-05-15T18:46:01Z

Ok, I started to write the code and then remembered more: Avoiding casts means we need an extra scratch local for the condition. We can avoid constant increases in roundtrips by detecting the case of a local.get there, but we'd need to be careful of a local.get of a scratch local (edit: and StackIR stuff as in the other PR). Overall it is more complicated.

I think if we end up with types that cannot be cast in the future then we'd have no choice but to make that change, but for now it seems best not to, to me.

tlively

LGTM rebased on the landed stringview PRs.

tlively · 2024-05-15T19:17:33Z

+
+ assert(curr->type.hasRef());
+
+ auto emitCast = [&](Type to, Type from) {


Since the stringview PR has now landed, there's no need to supply the from type anymore, so this can be simplified a bit. I guess we still need the unrefinedType in the brIfsNeedingHandling map for the scratch locals, though.

Yeah, from can go away but we do still need to know the unrefined type because the scratch locals are of that type.

kripken added 30 commits April 10, 2024 17:03

uilds
f835f74

builds
0ae495f

todo
7c7ff86

tests
a1075d2

test
a787489

fix
497d0b8

same
097970b

fix
23f006c

test
1233da1

fix
a5e6037

test notes
47bdc44

fix
d152156

fix
e285a11

fix
205c4e9

fix
cfdd78f

test, sad
585f9c8

typo
57879cf

thinking out loud
235af4f

work
90dd4b8

yolo
532c214

work
4342d57

work
e8d5f6b

work
96c2bce

work?
631adb9

work?
f829de3

work?
3f0ba33

work
bb762f9

fix
bd82ccd

fix
f6c5c19

work
d73efd9

kripken added 11 commits May 14, 2024 14:59

fix
55a13fd

text
f663842

fix
70f4812

fix
ad87777

test
f8e13e0

fix
4d40f31

fix
20df949

test
cd94ea1

text
d4294df

text
bc4f68e

text
f572f54

tlively reviewed May 15, 2024
View reviewed changes

tlively approved these changes May 15, 2024
View reviewed changes

kripken added 5 commits May 15, 2024 12:51

Merge remote-tracking branch 'origin/main' into br_if.alternative
5301467

optimize
5a5a2af

Remove some complexity now that visitRefCast is simpler
2e79f34

merge oops
1d6497c

Merge remote-tracking branch 'origin/main' into br_if.alternative
03eb588

kripken mentioned this pull request May 15, 2024
Make br_if with a value's type match the spec #6390
Closed

kripken enabled auto-merge (squash) May 15, 2024 23:31

kripken merged commit e5f2edf into WebAssembly:mainMay 16, 2024

kripken deleted the br_if.alternative branch May 16, 2024 00:11

vouillon mentioned this pull request May 30, 2024
[BUG] Compiling function failed with type error in range pattern case ocaml-wasm/wasm_of_ocaml#41
Closed

gkdn mentioned this pull request Aug 31, 2024
stringconsts gkdn/binaryen#1
Closed

		auto localIndex = scratchLocals[t] + scratchTypeUses[t];
		scratchTypeUses[t]++;

		scratchTypeUses[t]--;
		auto localIndex = scratchLocals[t] + scratchTypeUses[t];

	scratchTypeUses[t]--;
	auto localIndex = scratchLocals[t] + scratchTypeUses[t];
	auto localIndex = scratchLocals[t] + --scratchTypeUses[t];


		assert(curr->type.hasRef());

		auto emitCast = [&](Type to, Type from) {

Uh oh!

Conversation

kripken commented Apr 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented May 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripkenMay 15, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

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 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!

kripken commented May 15, 2024

Uh oh!

tlively commented May 15, 2024

Uh oh!

kripken commented May 15, 2024

Uh oh!

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

Uh oh!

Uh oh!

tlively 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kripken commented Apr 17, 2024•
edited
Loading

kripkenMay 15, 2024•
edited
Loading

kripken commented May 15, 2024•
edited
Loading