Add an Intrinsics mechanism, and a call.if.used intrinsic #4126

kripken · 2021-09-07T20:21:42Z

An "intrinsic" is modeled as a call to an import. We could also add new
IR things for them, but that would take more work and lead to less
clear errors in other tools if they try to read a binary using such a
nonstandard extension. Curious if there are other ideas here though!

A first intrinsic is added here, call.if.used. This is basically the same
as call_ref except that the optimizer is free to remove the call if the
result is not used. That is, if foo() returns an int, then we normally
cannot optimize way foo(); (a call whose result is not used). But
if we write call.if.used(foo); then the optimizer can remove that.

A lowering pass for intrinsics is provided. Rather than automatically
lower them to normal wasm at the end of optimizations, the user must
call that pass explicitly. A typical workflow might be

-O --intrinsic-lowering -O

That optimizes with the intrinsic present - perhaps removing calls
thanks to it - then lowers it into normal wasm - it turns into a call_ref -
and then optimizes further, which would turns the call_ref into a
direct call, potentially inline, etc.

The purpose of call.if.used is to allow us to optimize away calls to
things that have unnecessary side effects. For example, if we have a
method getSingletonFoo() that returns a Foo object in j2cl then
we might have code like this:

function bar() {
  var foo = getSingletonFoo();
  if (0) foo.something();
}

After local optimizations we end up with

function bar() {
  getSingletonFoo();
}

Imagine that getSingletonFoo() creates a Foo on the first call,
so it has side effects. But, the wasm producer knows that it is ok
to not run those side effects if the value is not used - the object
will be created later (or never), and that is ok in terms of the
language semantics.

aheejin

I see, thanks for the explanation!

kripken · 2021-09-08T22:16:42Z

I pushed an update to replace this with call.without.effects as suggested. Turns out that's even simpler to implement, so my earlier guess was wrong that it would be more work...

src/ir/intrinsics.h

tlively · 2021-09-08T22:58:04Z

Would something like ignore.side.effects that can wrap any expression be more versatile?

@tlively had the same idea offline, coincidentially Yeah, I think you are both probably right, that is actually better than this. I tried to do the "minimal" thing here, but side effects is the real issue.

@aheejin's suggested ignore.side.effects is actually more general than what I suggested, which IIRC was essentially the current call.without.effects. I think this PR would be simpler if it implemented @aheejin's suggestion, i.e. an intrinsic that could wrap any expression (including any kind of call). Specifically, the lowering would be simpler because the intrinsic would simply be replaced with its single operand and validation would be simplified because it would take the type of its single operand. Function references would not be necessary at all.

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

kripken · 2021-09-09T16:33:58Z

Oh, sorry, I did not understand that specific proposal, then.

There are composition issues with that idea, if I understand it correctly. E.g.

(local.set $x
  (ignore.side.effects
    (call $foo)))

would not be equivalent to

(local.set $temp
  (call $foo))
(local.set $x
  (ignore.side.effects
    (local.get $temp)))

(flatten converts from the former to the latter, and simplify-locals in reverse).

And also things like

(ignore.side.effects
  (call $foo))

?

(ignore.side.effects
  (block (result ..)
    (call $foo)))

Does it check effects on the block, or on all nested children? If just the block, then that's wrong; if all nested children, then we can hit a simplify-locals issue like from earlier:

(local.set $x
  (call $bar))
(ignore.side.effects
  (call $foo
    (local.get $x)))

?

(ignore.side.effects
  (call $foo
    (call $bar)))

There isn't a good way to force two expressions to remain "attached", that is, stay as the direct child of the other (and also no way to prevent adding more nested children in addition). More generally, the "meaning" of a node should not depend on "context" around it (with the exceptions of breaks).

tlively · 2021-09-09T23:35:59Z

Oof, that's unfortunate. Thanks for the clear examples.

aheejin · 2021-09-10T10:38:00Z

src/ir/intrinsics.h

+  bool isCallWithoutEffects(Function* func);
+  Call* isCallWithoutEffects(Expression* curr);


Should this be a class with non-static member functions? These functions don't seem to refer to the module member...

It's used here: https://github.com/WebAssembly/binaryen/pull/4126/files#diff-4e0ac873d5e741220507e824e0b69ef79057f2ff1de3117afeb9ceb84cd4f714R33 to get the Function from the module using its name.

aheejin · 2021-09-10T10:38:00Z

src/passes/Intrinsics.cpp

+        auto* func = getModule()->getFunction(name);
+        if (!HeapType::isSubType(func->type, refFunc->type.getHeapType())) {
+          Fatal() << "call.without.effects is not of a subtype of the called "
+                     "function ("
+                  << name << ")\n";
+        }


Isn't this a part of the validation of RefFunc, separate from call.without.effects, which should have been done in the validator already?

Good catch! I wanted to check that the type of the call.without.effects is the right one. It should be identical to the function reference's, ignoring the last parameter (which is the function reference), but this code does not do that...

Actually checking this would be a bunch more code, so I think maybe I'll just remove it, and leave it to the validator. I'd hoped for a clearer error message here, but let's see if it's a problem in practice.

aheejin · 2021-09-10T10:38:00Z

src/passes/Intrinsics.cpp

+        if (target->type.isBasic()) {
+          Fatal() << "Cannot emit call_ref for call.without.effects without "
+                     "a specialized function type";
+        }


Shouldn't this be in the validation of call.without.effects instead?

aheejin · 2021-09-10T10:38:00Z

test/lit/passes/vacuum-intrinsics.wast

@@ -0,0 +1,233 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
+;; RUN: wasm-opt %s --vacuum -all -S -o - | filecheck %s


Now we don't have any changes in Vacuum.cpp, then where do we do these optimizations?

Just by seeing that the call has no side effects, vacuum is able to remove more things. It will remove a drop of anything without side effects, for example.

kripken added 30 commits Aug 31, 2021

wip

2e3d9f3

format

47788c8

update a test

e670fcf

wip

c4ffd09

fix

1785ee9

work

e1f7ab1

fix

8143c8d

builds

c25b937

opt

deb6c4a

work

eaf790f

test

9763b0f

fix

ba877c5

fix

94c8e6e

test

e8bf271

work

4346e1b

waka

290b42b

Merge remote-tracking branch 'origin/main' into intrin

82d0bd3

clean

abdba9f

more

ca60836

fix

5c4358b

test

2f64508

readme

5169800

Merge remote-tracking branch 'origin/main' into intrin

b381460

comments

6e17652

missing file

8dfcab0

better

32cb7cc

good test that fails

d90842c

fix

12133b3

test

b71b257

format

6d5507e

kripken added 2 commits Sep 8, 2021

Merge remote-tracking branch 'origin/intrin' into intrin

e0f4223

call.if.used => call.without.effects

27038a6

aheejin approved these changes Sep 8, 2021

View changes

kripken added 9 commits Sep 8, 2021

cleaner

741ea85

fixes?

5e02a8f

update tests

9005ea0

test

f9c2e45

cleanup

6bec10d

clear error

e3a6c81

mention error handling

7ee7ece

fixes

814ed3d

cleaner

3b86059

format

03c148f

tlively reviewed Sep 8, 2021

View changes

src/ir/intrinsics.h Outdated Show resolved Hide resolved

src/ir/intrinsics.h Outdated Show resolved Hide resolved

kripken and others added 2 commits Sep 9, 2021

Update src/ir/intrinsics.h

834db19

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

Update src/ir/intrinsics.h

950cdbc

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

tlively approved these changes Sep 9, 2021

View changes

aheejin reviewed Sep 10, 2021

View changes

kripken added 2 commits Sep 10, 2021

Remove extra and incorrect validation

86473c5

format

caba1d9

aheejin approved these changes Sep 10, 2021

View changes

remove a validation test for the validation code that was removed

7ddfe6d

kripken enabled auto-merge (squash) Sep 10, 2021

kripken deleted the intrin branch Sep 10, 2021

Aug	SEP	Oct
	12
2020	2021	2022

WebAssembly / binaryen Public

Add an Intrinsics mechanism, and a call.if.used intrinsic #4126

Add an Intrinsics mechanism, and a call.if.used intrinsic #4126

kripken commented Sep 7, 2021

aheejin left a comment

kripken commented Sep 8, 2021

tlively commented Sep 8, 2021

kripken commented Sep 9, 2021

tlively commented Sep 9, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

		bool isCallWithoutEffects(Function* func);
		Call* isCallWithoutEffects(Expression* curr);

		@@ -0,0 +1,233 @@
		;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
		;; RUN: wasm-opt %s --vacuum -all -S -o - \| filecheck %s

WebAssembly / binaryen Public

Add an Intrinsics mechanism, and a call.if.used intrinsic #4126

Add an Intrinsics mechanism, and a call.if.used intrinsic #4126

Conversation

kripken commented Sep 7, 2021

aheejin left a comment

kripken commented Sep 8, 2021

tlively commented Sep 8, 2021

kripken commented Sep 9, 2021

tlively commented Sep 9, 2021

This comment has been minimized.

aheejin Sep 10, 2021 Member

This comment has been minimized.

kripken Sep 10, 2021 Author Member

This comment has been minimized.

aheejin Sep 10, 2021 Member

This comment has been minimized.

kripken Sep 10, 2021 Author Member

This comment has been minimized.

aheejin Sep 10, 2021 Member

This comment has been minimized.

aheejin Sep 10, 2021 Member

This comment has been minimized.

kripken Sep 10, 2021 Author Member

aheejin Sep 10, 2021
Member

kripken Sep 10, 2021
Author Member

aheejin Sep 10, 2021
Member

kripken Sep 10, 2021
Author Member

aheejin Sep 10, 2021
Member

aheejin Sep 10, 2021
Member

kripken Sep 10, 2021
Author Member