Always inline trivial calls that always shrink

[osa1]

Currently a "trivial call" is a function that just calls another function.

(func $foo ...
  (call $target ...))

Where the arguments to $target are all flat instructions like local.get, or
consts.

Currently we inline these functions always only when not optimizing for code
size.

When optimizing for code size, these functions can always be inlined when

1. The arguments to $target are all function argument locals.
2. Each local is used once.
3. In the order they appear in $foo's signature.

When these hold, inlining $foo never increases code size as it doesn't cause
introducing more locals (or drops etc.) at the call sites.

$foo above when these hold looks like this:

(func $foo (param $arg1 ...) (param $arg2 ...)
  (call $target (local.get $arg1) (local.get $arg2)))

Update FunctionInfo type and FunctionInfoScanner to annotate functions with
more detailed "trivial call" information that also contains whether inlining
shrinks code size.

If a function shrinks when inlined always inline it even with -Os.

Otherwise inline it as before, i.e. when not optimizing for code size.

[osa1]

This is a little bit verbose. Should we run a simple pass to clean this up? (eliminate blocks and locals)

[kripken]

We have --inlining-optimizing for that, and that is what the normal pass pipeline uses. --inlining without optimizing is mainly useful for debugging and tests.

[osa1]

Thanks. --inlining-optimizing simplifies the wat in this test quite a bit. Should I use it in these tests, or do we prefer testing smaller units of code with just --inlining?

[kripken]

Suggested change

	// Whether a function just calls another function with only `local.get`s as
	// arguments.
	// Whether a function just calls another function with only trivial
	// (`local.get` or constant) arguments.

I think handling constants is important too, and the old code did so (by the heuristic that not having children - that's not precise, but it's probably good enough?)

[osa1]

Wouldn't that increase code size when you have more than one call site, by duplicating the constants at the call sites?

Where did the old code do this? I'm trying

(module
 (type $0 (func (param i32 i32 i32)))

 (type $1 (func))

 (type $2 (func))

 (import "env" "foo" (func $imported-foo (type $0) (param i32 i32 i32)))

 (func $call-foo (type $1)
  (call $imported-foo
   (i32.const 1)
   (i32.const 2)
   (i32.const 3)))

 (func $main (type $2)
  (call $call-foo)
  (call $call-foo)
  (call $call-foo)))

with wasm-opt -all --inlining -S -o - < test.wat but it doesn't inline call-foo. (trying with main branch)

[osa1]

Ah, I see the old code where we check the size now. However I'm unable to make wasm-opt inline function calls with const arguments, using the main branch. I'll investigate.

I think the point about binary size increase still applies though..

[osa1]

I figured it out, it actually works the same way as before. I updated the comments to clarify, and added a test. PTAL.

[kripken]

Unresolving as I don't follow. Are you saying we did not inline constants before, that is, the old comment about us doing so was wrong? If so, where was the bug, as we measured size, and the size of a const is 1, same as local.get?

[osa1]

The old code that made us inline const arguments is still there and running as before, on lines 268-271 on the new file. I added a test to check const argument inlining. It works the same way before and after this PR.

The way it works in new code is that if we know that the code definitely shrinks when inlined, we have an early return. If we don't know that, then the old code that checks the size still runs as before and marks the function as MayNotShrink. In which case we only inline with -O3 as before.

[kripken]

Ok, sounds good. In that case, the comments on new lines 77-83 can be updated to reflect that not only local.gets are allowed.

[kripken]

Oh, and also where this comment is, 70-71.

[osa1]

I updated only the MayNotShrink documentation:

• The TrivialCall type documentation does not mention the argument types:

  // Whether a function just calls another function in a way that always shrinks
  // when the calling function is inlined.
  

  The argument types (constant or local) depend on the values so they're explained in the documentations of variants.

• The Shrinks variant is only generated when the arguments are all locals. We don't generate it for constant because constants can be a a few bytes and increase binary sizes. The documentation:

  // Function just calls another function, with `local.get`s as arguments, and
  // with each `local` is used exactly once, and in the order they appear in the
  // argument list.
  //
  // In this case, inlining the function generates smaller code, and it is also
  // good for runtime.
  Shrinks,
  

  This is in sync with the code.

• The MayNotShrink variant did not mention constants before, I now updated it to mention constants. Current documentation:

  // Function just calls another function, but maybe with constant arguments, or
  // maybe some locals are used more than once. In these cases code size does
  // not always shrink: at the call sites, omitted locals can create `drop`
  // instructions, a local used multiple times can create new locals, and
  // encoding of constants may be larger than just a `local.get` with a small
  // index. In these cases we still want to inline with `-O3`, but the code size
  // may increase when inlined.
  MayNotShrink,
  

PTAL.

[osa1]

@kripken It looks like this revealed an existing undefined behavior in this line:

binaryen/src/passes/OptimizeInstructions.cpp

Line 3612 in 8470f1b

maskLeft = (1ULL << leftMaxBits) - 1;

Apparently leftMaxBits can be 64, there are a few code paths in getMaxBits that return 64, e.g.

binaryen/src/ir/bits.h

Lines 267 to 270 in 8470f1b

	case RotLInt64:
	case RotRInt64:
	case SubInt64:
	return 64;

Is this a blocker? It looks like an existing issue that getMaxBits returns 64, which is used in a left shift.

[tlively]

I've also been running into that UB (which is not related to any of the PRs it shows up on) and I've fixed it in #7676.

[osa1]

@kripken I found a test that checks inlining with -O3 when arguments are all constants:

binaryen/test/lit/passes/inlining_optimize-level=3.wast

Lines 550 to 558 in da6c937

	(func $middle2 (param $x i32) (param $y i32) (param $z i32)
	;; Also trivial, even though the order of params is different and we have a
	;; const.
	(call $top
	(local.get $z)
	(i32.const 42)
	(local.get $x)
	)
	)

Should I remove this new test?