Improve inlining "heavyweight" (#3085)

Split that mode into an option to check for loops (which indicate a function
is "heavy") and a constant check for having calls. The case of calls is
different as we would need more logic to avoid infinite recursion if we are
willing to inling functions with calls.

Practically, this renames allowHeavyweight to allowFunctionsWithLoops.

12 files changed, 214 insertions, 126 deletions

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index db9258d59..101af5172 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -3528,12 +3528,12 @@ void BinaryenSetOneCallerInlineMaxSize(BinaryenIndex size) {
   globalPassOptions.inlining.oneCallerInlineMaxSize = size;
 }
 
-int BinaryenGetAllowHeavyweight(void) {
-  return globalPassOptions.inlining.allowHeavyweight;
+int BinaryenGetAllowInliningFunctionsWithLoops(void) {
+  return globalPassOptions.inlining.allowFunctionsWithLoops;
 }
 
-void BinaryenSetAllowHeavyweight(int enabled) {
-  globalPassOptions.inlining.allowHeavyweight = enabled;
+void BinaryenSetAllowInliningFunctionsWithLoops(int enabled) {
+  globalPassOptions.inlining.allowFunctionsWithLoops = enabled;
 }
 
 void BinaryenModuleRunPasses(BinaryenModuleRef module,
diff --git a/src/binaryen-c.h b/src/binaryen-c.h
index 6239befd2..893d44e3c 100644
--- a/src/binaryen-c.h
+++ b/src/binaryen-c.h
@@ -2124,13 +2124,13 @@ BINARYEN_API BinaryenIndex BinaryenGetOneCallerInlineMaxSize(void);
 // Applies to all modules, globally.
 BINARYEN_API void BinaryenSetOneCallerInlineMaxSize(BinaryenIndex size);
 
-// Gets whether heavyweight functions are allowed to be inlined.
+// Gets whether functions with loops are allowed to be inlined.
 // Applies to all modules, globally.
-BINARYEN_API int BinaryenGetAllowHeavyweight(void);
+BINARYEN_API int BinaryenGetAllowInliningFunctionsWithLoops(void);
 
-// Sets whether heavyweight functions are allowed to be inlined.
+// Sets whether functions with loops are allowed to be inlined.
 // Applies to all modules, globally.
-BINARYEN_API void BinaryenSetAllowHeavyweight(int enabled);
+BINARYEN_API void BinaryenSetAllowInliningFunctionsWithLoops(int enabled);
 
 // Runs the specified passes on the module. Uses the currently set global
 // optimize and shrink level.
diff --git a/src/js/binaryen.js-post.js b/src/js/binaryen.js-post.js
index 057252f78..4e673ab4b 100644
--- a/src/js/binaryen.js-post.js
+++ b/src/js/binaryen.js-post.js
@@ -3031,13 +3031,13 @@ Module['setOneCallerInlineMaxSize'] = function(size) {
 };
 
 // Gets the value which allow inline functions that are not "lightweight".
-Module['getAllowHeavyweight'] = function() {
-  return Boolean(Module['_BinaryenGetAllowHeavyweight']());
+Module['getAllowInliningFunctionsWithLoops'] = function() {
+  return Boolean(Module['_BinaryenGetAllowInliningFunctionsWithLoops']());
 };
 
 // Sets the value which allow inline functions that are not "lightweight".
-Module['setAllowHeavyweight'] = function(value) {
-  Module['_BinaryenSetAllowHeavyweight'](value);
+Module['setAllowInliningFunctionsWithLoops'] = function(value) {
+  Module['_BinaryenSetAllowInliningFunctionsWithLoops'](value);
 };
 
 // Expression wrappers
diff --git a/src/pass.h b/src/pass.h
index f6dd52128..a3ee41d61 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -77,9 +77,9 @@ struct InliningOptions {
   // Function size which we inline when there is only one caller.
   // FIXME: this should logically be higher than flexibleInlineMaxSize.
   Index oneCallerInlineMaxSize = 15;
-  // Allow inlining of functions that are not "lightweight" in the sense the
-  // inlining pass estimates.
-  bool allowHeavyweight = false;
+  // Loops usually mean the function does heavy work, so the call overhead
+  // is not significant and we do not inline such functions by default.
+  bool allowFunctionsWithLoops = false;
 };
 
 struct PassOptions {
diff --git a/src/passes/Inlining.cpp b/src/passes/Inlining.cpp
index c8c8a3f7b..d7f132879 100644
--- a/src/passes/Inlining.cpp
+++ b/src/passes/Inlining.cpp
@@ -48,13 +48,15 @@ namespace wasm {
 struct FunctionInfo {
   std::atomic<Index> refs;
   Index size;
-  std::atomic<bool> lightweight;
+  bool hasCalls;
+  bool hasLoops;
   bool usedGlobally; // in a table or export
 
   FunctionInfo() {
     refs = 0;
     size = 0;
-    lightweight = true;
+    hasCalls = false;
+    hasLoops = false;
     usedGlobally = false;
   }
 
@@ -79,12 +81,16 @@ struct FunctionInfo {
         size <= options.inlining.oneCallerInlineMaxSize) {
       return true;
     }
-    // more than one use, so we can't eliminate it after inlining,
+    // More than one use, so we can't eliminate it after inlining,
     // so only worth it if we really care about speed and don't care
-    // about size, and if it's lightweight so a good candidate for
-    // speeding us up.
+    // about size. First, check if it has calls. In that case it is not
+    // likely to speed us up, and also if we want to inline such
+    // functions we would need to be careful to avoid infinite recursion.
+    if (hasCalls) {
+      return false;
+    }
     return options.optimizeLevel >= 3 && options.shrinkLevel == 0 &&
-           (lightweight || options.inlining.allowHeavyweight);
+           (!hasLoops || options.inlining.allowFunctionsWithLoops);
   }
 };
 
@@ -101,16 +107,16 @@ struct FunctionInfoScanner
   }
 
   void visitLoop(Loop* curr) {
-    // having a loop is not lightweight
-    (*infos)[getFunction()->name].lightweight = false;
+    // having a loop
+    (*infos)[getFunction()->name].hasLoops = true;
   }
 
   void visitCall(Call* curr) {
     // can't add a new element in parallel
     assert(infos->count(curr->target) > 0);
     (*infos)[curr->target].refs++;
-    // having a call is not lightweight
-    (*infos)[getFunction()->name].lightweight = false;
+    // having a call
+    (*infos)[getFunction()->name].hasCalls = true;
   }
 
   void visitRefFunc(RefFunc* curr) {
diff --git a/src/tools/optimization-options.h b/src/tools/optimization-options.h
index ff43b7213..72f478329 100644
--- a/src/tools/optimization-options.h
+++ b/src/tools/optimization-options.h
@@ -165,12 +165,12 @@ struct OptimizationOptions : public ToolOptions {
              passOptions.inlining.oneCallerInlineMaxSize =
                static_cast<Index>(atoi(argument.c_str()));
            })
-      .add("--inline-heavyweight-functions",
-           "-ihf",
-           "Allow inlining heavyweight functions",
+      .add("--inline-functions-with-loops",
+           "-ifwl",
+           "Allow inlining functions with loops",
            Options::Arguments::Zero,
            [this](Options* o, const std::string&) {
-             passOptions.inlining.allowHeavyweight = true;
+             passOptions.inlining.allowFunctionsWithLoops = true;
            })
       .add("--ignore-implicit-traps",
            "-iit",
diff --git a/test/binaryen.js/inlining-options.js b/test/binaryen.js/inlining-options.js
index c2913e83d..bc716bf91 100644
--- a/test/binaryen.js/inlining-options.js
+++ b/test/binaryen.js/inlining-options.js
@@ -10,6 +10,6 @@ console.log("// oneCallerInlineMaxSize=" + binaryen.getOneCallerInlineMaxSize())
 binaryen.setOneCallerInlineMaxSize(33);
 assert(binaryen.getOneCallerInlineMaxSize() == 33);
 
-console.log("// allowHeavyweight=" + binaryen.getAllowHeavyweight());
-binaryen.setAllowHeavyweight(true);
-assert(binaryen.getAllowHeavyweight() == true);
+console.log("// allowInliningFunctionsWithLoops=" + binaryen.getAllowInliningFunctionsWithLoops());
+binaryen.setAllowInliningFunctionsWithLoops(true);
+assert(binaryen.getAllowInliningFunctionsWithLoops() == true);
diff --git a/test/binaryen.js/inlining-options.js.txt b/test/binaryen.js/inlining-options.js.txt
index 8dc7d5cf1..b32073a1a 100644
--- a/test/binaryen.js/inlining-options.js.txt
+++ b/test/binaryen.js/inlining-options.js.txt
@@ -1,4 +1,4 @@
 // alwaysInlineMaxSize=2
 // flexibleInlineMaxSize=20
 // oneCallerInlineMaxSize=15
-// allowHeavyweight=false
+// allowInliningFunctionsWithLoops=false
diff --git a/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.txt b/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.txt
new file mode 100644
index 000000000..023709819
--- /dev/null
+++ b/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.txt
@@ -0,0 +1,95 @@
+(module
+ (type $i32_=>_i32 (func (param i32) (result i32)))
+ (memory $memory 0)
+ (export "fib" (func $fib))
+ (export "looped" (func $looped))
+ (export "t0" (func $looped))
+ (export "t1" (func $t1))
+ (export "t2" (func $t2))
+ (export "t3" (func $t3))
+ (export "memory" (memory $memory))
+ (func $fib (; has Stack IR ;) (param $0 i32) (result i32)
+  (if
+   (i32.le_s
+    (local.get $0)
+    (i32.const 2)
+   )
+   (return
+    (local.get $0)
+   )
+  )
+  (i32.add
+   (call $fib
+    (i32.sub
+     (local.get $0)
+     (i32.const 1)
+    )
+   )
+   (call $fib
+    (i32.sub
+     (local.get $0)
+     (i32.const 2)
+    )
+   )
+  )
+ )
+ (func $looped (; has Stack IR ;) (param $0 i32) (result i32)
+  (loop $L0
+   (if
+    (i32.ge_s
+     (local.get $0)
+     (i32.const 0)
+    )
+    (block
+     (local.set $0
+      (i32.sub
+       (local.get $0)
+       (i32.const 1)
+      )
+     )
+     (br $L0)
+    )
+   )
+  )
+  (local.get $0)
+ )
+ (func $t1 (; has Stack IR ;) (param $0 i32) (result i32)
+  (local.set $0
+   (i32.add
+    (local.get $0)
+    (i32.const 1)
+   )
+  )
+  (loop $L0
+   (if
+    (i32.ge_s
+     (local.get $0)
+     (i32.const 0)
+    )
+    (block
+     (local.set $0
+      (i32.sub
+       (local.get $0)
+       (i32.const 1)
+      )
+     )
+     (br $L0)
+    )
+   )
+  )
+  (local.get $0)
+ )
+ (func $t2 (; has Stack IR ;) (param $0 i32) (result i32)
+  (call $fib
+   (local.get $0)
+  )
+ )
+ (func $t3 (; has Stack IR ;) (param $0 i32) (result i32)
+  (call $fib
+   (i32.add
+    (local.get $0)
+    (i32.const 1)
+   )
+  )
+ )
+)
diff --git a/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.wast b/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.wast
new file mode 100644
index 000000000..b1085b922
--- /dev/null
+++ b/test/passes/O3_inline-functions-with-loops_flexible-inline-max-function-size=30.wast
@@ -0,0 +1,80 @@
+(module
+  (type $t0 (func (param i32) (result i32)))
+  (func $fib (export "fib") (type $t0) (param $p0 i32) (result i32)
+    (if $I0
+      (i32.le_s
+        (local.get $p0)
+        (i32.const 2)
+      )
+      (then
+        (return
+          (local.get $p0)
+        )
+      )
+    )
+    (i32.add
+      (call $fib
+        (i32.sub
+          (local.get $p0)
+          (i32.const 1)
+        )
+      )
+      (call $fib
+        (i32.sub
+          (local.get $p0)
+          (i32.const 2)
+        )
+      )
+    )
+  )
+  (func $looped (export "looped") (type $t0) (param $p0 i32) (result i32)
+    (loop $L0
+      (if $I1
+        (i32.ge_s
+          (local.get $p0)
+          (i32.const 0)
+        )
+        (then
+          (local.set $p0
+            (i32.sub
+              (local.get $p0)
+              (i32.const 1)
+            )
+          )
+          (br $L0)
+        )
+      )
+    )
+    (local.get $p0)
+  )
+
+  (func $t0 (export "t0") (type $t0) (param $p0 i32) (result i32)
+    (call $looped
+      (local.get $p0)
+    )
+  )
+
+  (func $t1 (export "t1") (type $t0) (param $p0 i32) (result i32)
+    (call $looped
+      (i32.add
+        (local.get $p0)
+        (i32.const 1)
+      )
+    )
+  )
+  (func $t2 (export "t2") (type $t0) (param $p0 i32) (result i32)
+    (call $fib
+      (local.get $p0)
+    )
+  )
+
+  (func $t3 (export "t3") (type $t0) (param $p0 i32) (result i32)
+    (call $fib
+      (i32.add
+        (local.get $p0)
+        (i32.const 1)
+      )
+    )
+  )
+  (memory $memory (export "memory") 0)
+)
diff --git a/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.txt b/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.txt
deleted file mode 100644
index 685ba4513..000000000
--- a/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.txt
+++ /dev/null
@@ -1,54 +0,0 @@
-(module
- (type $i32_=>_i32 (func (param i32) (result i32)))
- (memory $memory 0)
- (export "test" (func $test))
- (export "t0" (func $test))
- (export "t1" (func $t1))
- (export "memory" (memory $memory))
- (func $test (; has Stack IR ;) (param $0 i32) (result i32)
-  (loop $L0
-   (if
-    (i32.ge_s
-     (local.get $0)
-     (i32.const 0)
-    )
-    (block
-     (local.set $0
-      (i32.sub
-       (local.get $0)
-       (i32.const 1)
-      )
-     )
-     (br $L0)
-    )
-   )
-  )
-  (local.get $0)
- )
- (func $t1 (; has Stack IR ;) (param $0 i32) (result i32)
-  (local.set $0
-   (i32.add
-    (local.get $0)
-    (i32.const 1)
-   )
-  )
-  (loop $L0
-   (if
-    (i32.ge_s
-     (local.get $0)
-     (i32.const 0)
-    )
-    (block
-     (local.set $0
-      (i32.sub
-       (local.get $0)
-       (i32.const 1)
-      )
-     )
-     (br $L0)
-    )
-   )
-  )
-  (local.get $0)
- )
-)
diff --git a/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.wast b/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.wast
deleted file mode 100644
index ec8e5bfe5..000000000
--- a/test/passes/O3_inline-heavyweight-functions_flexible-inline-max-function-size=30.wast
+++ /dev/null
@@ -1,39 +0,0 @@
-(module
-  (type $t0 (func (param i32) (result i32)))
-  (func $test (export "test") (type $t0) (param $p0 i32) (result i32)
-    (loop $L0
-      (if $I1
-        (i32.ge_s
-          (local.get $p0)
-          (i32.const 0)
-        )
-        (then
-          (local.set $p0
-            (i32.sub
-              (local.get $p0)
-              (i32.const 1)
-            )
-          )
-          (br $L0)
-        )
-      )
-    )
-    (local.get $p0)
-  )
-
-  (func $t0 (export "t0") (type $t0) (param $p0 i32) (result i32)
-    (call $test
-      (local.get $p0)
-    )
-  )
-
-  (func $t1 (export "t1") (type $t0) (param $p0 i32) (result i32)
-    (call $test
-      (i32.add
-        (local.get $p0)
-        (i32.const 1)
-      )
-    )
-  )
-  (memory $memory (export "memory") 0)
-)