Bug 1467415 - Importable mutable globals break alias analysis. r=lth a=lizzard
authorJulian Seward <jseward@acm.org>
Fri, 29 Jun 2018 11:49:35 +0200
changeset 477837 4f29d9433d69f95e01796fd687f7035c267cb62c
parent 477836 43f3051e9efa7b39531644a7b2f82f62f7b0eb4e
child 477838 89c03b4aab112da9d4a0ac3a8d0e4be32315f366
push id9444
push userarchaeopteryx@coole-files.de
push dateSat, 07 Jul 2018 21:01:06 +0000
treeherdermozilla-beta@3c8ab5a011e3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth, lizzard
bugs1467415
milestone62.0
Bug 1467415 - Importable mutable globals break alias analysis. r=lth a=lizzard Currently, do-these-references-alias queries for Wasm global variable accesses are handled incorrecty. We fail to identify the case where both references are indirect (that is, imported) as possibly-aliasing. This can lead to incorrect code generation in some pretty simple cases involving globals, when compiling Wasm via IonMonkey. This commit fixes that. It also adds clarifying comments to MWasmLoadGlobalVar::congruentTo and MWasmLoadGlobalVar::valueHash pertaining to equivalance-of-loaded-values assessments.
js/src/jit-test/tests/wasm/regress/bug1467415.js
js/src/jit/MIR.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/wasm/regress/bug1467415.js
@@ -0,0 +1,29 @@
+
+// This exposed an alias analysis bug in the Wasm Ion machinery.  It should
+// compute 125 (88 + 37) but with the bug would compute 176 (88 + 88) instead.
+// See bug 1467415.
+
+let mt = `
+(module
+  (memory 1 1)
+  (data (i32.const 0) "\\01\\00\\00\\00\\01\\00\\00\\00\\01\\00\\00\\00")
+  (import "m" "g" (global (mut i32)))
+  (import "m" "h" (global (mut i32)))
+  (func (export "f") (result i32)
+    (local i32)
+    (local i32)
+    (block i32
+      (set_local 0 (get_global 0))
+      (block i32
+        (set_global 1 (i32.const 37))
+        (block i32
+          (set_local 1 (get_global 0))
+          (i32.add (get_local 0) (get_local 1)))))))
+`;
+
+let glob = new WebAssembly.Global({value:'i32', mutable:true}, 88);
+let module = new WebAssembly.Module(wasmTextToBinary(mt));
+let ins = new WebAssembly.Instance(module, {m:{g:glob, h:glob}});
+
+let shouldBe125 = ins.exports.f();
+assertEq(shouldBe125, 125);
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -5470,34 +5470,55 @@ MAsmJSLoadHeap::congruentTo(const MDefin
            congruentIfOperandsEqual(load);
 }
 
 MDefinition::AliasType
 MWasmLoadGlobalVar::mightAlias(const MDefinition* def) const
 {
     if (def->isWasmStoreGlobalVar()) {
         const MWasmStoreGlobalVar* store = def->toWasmStoreGlobalVar();
-        // Global variables can't alias each other or be type-reinterpreted.
-        return (store->globalDataOffset() == globalDataOffset_) ? AliasType::MayAlias :
-                                                                  AliasType::NoAlias;
+
+        // If they are both indirect, then we don't know what the
+        // indirections point at, so we must be conservative.
+        if (isIndirect_ && store->isIndirect())
+            return AliasType::MayAlias;
+
+        // If they are both direct, then we can disambiguate them by
+        // inspecting their offsets.
+        if (!isIndirect_ && !store->isIndirect())
+            return store->globalDataOffset() == globalDataOffset_
+                      ? AliasType::MayAlias : AliasType::NoAlias;
+
+        // Otherwise, one is indirect and the other isn't, so they can't
+        // alias.
+        return AliasType::NoAlias;
+
+        // We could do better here, in that: if both variables are indirect,
+        // but at least one of them is created in this module, then they
+        // can't alias.  That would require having a flag on globals to
+        // indicate which are imported.  See bug 1467415 comment 3,
+        // 4th rule.
     }
     return AliasType::MayAlias;
 }
 
 HashNumber
 MWasmLoadGlobalVar::valueHash() const
 {
+    // Same comment as in MWasmLoadGlobalVar::congruentTo() applies here.
     HashNumber hash = MDefinition::valueHash();
     hash = addU32ToHash(hash, globalDataOffset_);
     return hash;
 }
 
 bool
 MWasmLoadGlobalVar::congruentTo(const MDefinition* ins) const
 {
+    // We don't need to consider the isIndirect_ markings here, because
+    // equivalence of offsets implies equivalence of indirectness.
     if (ins->isWasmLoadGlobalVar())
         return globalDataOffset_ == ins->toWasmLoadGlobalVar()->globalDataOffset_;
     return false;
 }
 
 MDefinition*
 MWasmLoadGlobalVar::foldsTo(TempAllocator& alloc)
 {