[Branch Hints] Fix DuplicateFunctionElimination and function comparisons (#7715)

This adds metadata comparisons to `FunctionUtils::equal`: if two
functions differ in metadata, they are different, and should not be
merged in DuplicateFunctionElimination even if identical otherwise.

(In theory, they could be identical in all but branch hints, and called
from different places which cause the differing branch hints to
both be correct.)

Author: kripken

src/ir/function-utils.h

@@ -17,6 +17,7 @@
 #ifndef wasm_ir_function_h
 #define wasm_ir_function_h
 
+#include "ir/metadata.h"
 #include "ir/utils.h"
 #include "wasm.h"
 
@@ -37,10 +38,16 @@ inline bool equal(Function* left, Function* right) {
       return false;
     }
   }
-  if (!left->imported() && !right->imported()) {
-    return ExpressionAnalyzer::equal(left->body, right->body);
+  if (!metadata::equal(left, right)) {
+    return false;
+  }
+  if (left->imported() && right->imported()) {
+    return true;
+  }
+  if (left->imported() || right->imported()) {
+    return false;
   }
-  return left->imported() && right->imported();
+  return ExpressionAnalyzer::equal(left->body, right->body);
 }
 
 } // namespace wasm::FunctionUtils

src/ir/hashed.h

@@ -17,10 +17,11 @@
 #ifndef _wasm_ir_hashed_h
 #define _wasm_ir_hashed_h
 
+#include <functional>
+
 #include "ir/utils.h"
 #include "support/hash.h"
 #include "wasm.h"
-#include <functional>
 
 namespace wasm {
 
@@ -65,6 +66,8 @@ struct FunctionHasher : public WalkerPass<PostWalker<FunctionHasher>> {
     }
     hash_combine(digest,
                  ExpressionAnalyzer::flexibleHash(func->body, customHasher));
+    // TODO: Hash metadata (debug info, code annotations), though it would be
+    //       very rare to get a false collision for these reasons.
     return digest;
   }
 

src/ir/metadata.cpp

@@ -20,6 +20,25 @@
 
 namespace wasm::metadata {
 
+namespace {
+
+// List out instructions serially, so we can match them between the old and
+// new copies.
+//
+// This is not that efficient, and in theory we could copy this in the
+// caller context as the code is copied. However, we assume that most
+// functions have no metadata, so this is faster in that common case.
+struct Serializer
+  : public PostWalker<Serializer, UnifiedExpressionVisitor<Serializer>> {
+  Serializer(Expression* expr) { walk(expr); }
+
+  std::vector<Expression*> list;
+
+  void visitExpression(Expression* curr) { list.push_back(curr); }
+};
+
+} // anonymous namespace
+
 void copyBetweenFunctions(Expression* origin,
                           Expression* copy,
                           Function* originFunc,
@@ -30,21 +49,8 @@ void copyBetweenFunctions(Expression* origin,
     return;
   }
 
-  // List out instructions serially, so we can match them between the old and
-  // new copies.
-  //
-  // This is not that efficient, and in theory we could copy this in the
-  // caller context as the code is copied. However, we assume that most
-  // functions have no metadata, so this is faster in that common case.
-  struct Lister : public PostWalker<Lister, UnifiedExpressionVisitor<Lister>> {
-    std::vector<Expression*> list;
-    void visitExpression(Expression* curr) { list.push_back(curr); }
-  };
-
-  Lister originList;
-  originList.walk(origin);
-  Lister copyList;
-  copyList.walk(copy);
+  Serializer originList(origin);
+  Serializer copyList(copy);
 
   auto& originDebug = originFunc->debugLocations;
   auto& copyDebug = copyFunc->debugLocations;
@@ -70,4 +76,67 @@ void copyBetweenFunctions(Expression* origin,
   }
 }
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+
+// Given two expressions to use as keys, see if they have identical values (or
+// are both absent) in two maps.
+template<typename T, typename V>
+bool compare(Expression* a,
+             Expression* b,
+             const T& aMap,
+             const T& bMap,
+             const V defaultValue) {
+  auto aIter = aMap.find(a);
+  auto aItem = aIter != aMap.end() ? aIter->second : defaultValue;
+  auto bIter = bMap.find(b);
+  auto bItem = bIter != bMap.end() ? bIter->second : defaultValue;
+  return aItem == bItem;
+}
+
+bool equal(Function* a, Function* b) {
+  if (a->imported() && b->imported()) {
+    // No code metadata, and we don't yet store function-level metadata.
+    return true;
+  }
+  if (a->imported() || b->imported()) {
+    // See comment on declaration, we consider such a difference as making them
+    // unequal.
+    return false;
+  }
+
+  if (a->debugLocations.empty() && b->debugLocations.empty() &&
+      a->codeAnnotations.empty() && b->codeAnnotations.empty()) {
+    // Nothing to compare; no differences.
+    return true;
+  }
+
+  Serializer aList(a->body);
+  Serializer bList(b->body);
+
+  if (aList.list.size() != bList.list.size()) {
+    return false;
+  }
+
+  assert(aList.list.size() == bList.list.size());
+  for (Index i = 0; i < aList.list.size(); i++) {
+    if (!compare(aList.list[i],
+                 bList.list[i],
+                 a->debugLocations,
+                 b->debugLocations,
+                 Function::DebugLocation()) ||
+        !compare(aList.list[i],
+                 bList.list[i],
+                 a->codeAnnotations,
+                 b->codeAnnotations,
+                 Function::CodeAnnotation())) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+#pragma GCC diagnostic pop
+
 } // namespace wasm::metadata

src/ir/metadata.h

@@ -28,6 +28,13 @@ void copyBetweenFunctions(Expression* origin,
                           Function* originFunc,
                           Function* copyFunc);
 
+// Check if two functions have identical metadata. We consider differences like
+// one being imported and the other not, or having different numbers of
+// instructions, to mean they are not equal (as the meaning of comparisons
+// becomes hard in such cases, and the main use here is to compare metadata
+// after all else is known equal).
+bool equal(Function* a, Function* b);
+
 } // namespace wasm::metadata
 
 #endif // wasm_ir_metadata_h

src/wasm.h

@@ -2233,7 +2233,7 @@ class Function : public Importable {
   // Source maps debugging info: map expression nodes to their file, line, col,
   // symbol name.
   struct DebugLocation {
-    BinaryLocation fileIndex, lineNumber, columnNumber;
+    BinaryLocation fileIndex = -1, lineNumber = -1, columnNumber = -1;
     std::optional<BinaryLocation> symbolNameIndex;
     bool operator==(const DebugLocation& other) const {
       return fileIndex == other.fileIndex && lineNumber == other.lineNumber &&
@@ -2279,6 +2279,10 @@ class Function : public Importable {
     static const uint8_t NeverInline = 0;
     static const uint8_t AlwaysInline = 127;
     std::optional<uint8_t> inline_;
+
+    bool operator==(const CodeAnnotation& other) const {
+      return branchLikely == other.branchLikely && inline_ == other.inline_;
+    }
   };
 
   // Function-level annotations are implemented with a key of nullptr, matching