Bug 1797933, part 3 - add alias-set annotations for struct.{new,set,get} code. r=rhunt.

This patch adds alias set annotations to the MIR generated for
struct.{new,set,get}, so as to enable the existing GVN machinery to remove
duplicate loads of the OOL block pointer in the (static) presence of multiple
OOL field accesses to the same wasm object.

This is a bit tricky because we must ensure that neither an IL-data-field nor
OOL-data-field write to the object invalidate the OOL-block-pointer read.
Hence the OOL-block-pointer-field cannot be in the same alias set as either
the IL- nor OOL-data fields.  And so this patch adds three new alias-set
descriptors.

The implementation is straightforward and described in comments.

Because it is easy to mess up optimisation with incorrect alias set
descriptors, the `MWasm{Load,Store}Field*::New` methods heavily restrict
what descriptors they accept, via assertions.

Because those same MIR nodes are also used to implement exceptions, they also
accept `AliasSet::{Load,Store}(AliasSet::All)` ("no information") descriptors.
The exception-handling MIR is unaffected by this patch.

Differential Revision: https://phabricator.services.mozilla.com/D161254

js/src/jit-test/tests/wasm/gc/structs.js

@@ -634,3 +634,51 @@ var bad = new Uint8Array([0x00, 0x61, 0x73, 0x6d,
 
 assertErrorMessage(() => new WebAssembly.Module(bad),
                    WebAssembly.CompileError, /signature index references non-signature/);
+
+// Exercise alias-analysis code for struct access
+{
+    let txt =
+    `(module
+       (type $meh (struct))
+       (type $hasOOL (struct
+                      ;; In-line storage
+                      (field i64) (field i64)
+                      (field $ILnonref (mut i64)) (field $ILref (mut eqref))
+                      (field i64) (field i64) (field i64) (field i64)
+                      (field i64) (field i64) (field i64) (field i64)
+                      (field i64) (field i64) (field i64) (field i64)
+                      ;; Out-of-line storage (or maybe it starts earlier, but
+                      ;; definitely not after this point).
+                      (field $OOLnonref (mut i64)) (field $OOLref (mut eqref)))
+       )
+       (func (export "create") (result eqref)
+         (struct.new $hasOOL
+           (i64.const 1)    (i64.const 2)
+           (i64.const 9876) (ref.null $meh)
+           (i64.const 3)    (i64.const 4)   (i64.const 5)   (i64.const 6)
+           (i64.const 7)    (i64.const 8)   (i64.const 9)   (i64.const 10)
+           (i64.const 11)   (i64.const 12)  (i64.const 13)  (i64.const 14)
+           (i64.const 4321) (ref.null $meh))
+       )
+       ;; Write to an OOL field, then an IL field, then to an OOL field, so
+       ;; that we can at least check (from inspection of the optimised MIR)
+       ;; that the GVN+alias analysis causes the OOL block pointer not to be
+       ;; reloaded for the second OOL write.  First for non-ref fields ..
+       (func (export "threeSetsNonReffy") (param eqref)
+         (local (ref $hasOOL))
+         (local.set 1 (ref.cast $hasOOL (local.get 0)))
+         (struct.set $hasOOL 16 (local.get 1) (i64.const 1337)) ;; set $OOLnonref
+         (struct.set $hasOOL 2  (local.get 1) (i64.const 7331)) ;; set $ILnonref
+         (struct.set $hasOOL 16 (local.get 1) (i64.const 9009)) ;; set $OOLnonref
+       )
+       ;; and the same for ref fields.
+       (func (export "threeSetsReffy") (param eqref)
+         (local (ref $hasOOL))
+         (local.set 1 (ref.cast $hasOOL (local.get 0)))
+         (struct.set $hasOOL 17 (local.get 1) (ref.null $meh)) ;; set $OOLref
+         (struct.set $hasOOL 3  (local.get 1) (ref.null $meh)) ;; set $ILref
+         (struct.set $hasOOL 17 (local.get 1) (ref.null $meh)) ;; set $OOLref
+       )
+     )`;
+    let exports = wasmEvalText(txt).exports;
+}

js/src/jit/MIR.h

@@ -361,10 +361,19 @@ class AliasSet {
     // The fuzzilliHash slot
     FuzzilliHash = 1 << 19,
 
-    Last = FuzzilliHash,
+    // The WasmStruct::inlineData_[..] storage area
+    WasmStructInlineDataArea = 1 << 20,
+
+    // The WasmStruct::outlineData_ pointer only
+    WasmStructOutlineDataPointer = 1 << 21,
+
+    // The malloc'd block that WasmStruct::outlineData_ points at
+    WasmStructOutlineDataArea = 1 << 22,
+
+    Last = WasmStructOutlineDataArea,
 
     Any = Last | (Last - 1),
-    NumCategories = 20,
+    NumCategories = 23,
 
     // Indicates load or store.
     Store_ = 1 << 31
@@ -10724,15 +10733,21 @@ enum class MNarrowingOp : uint8_t { None, To16, To8 };
 class MWasmLoadField : public MUnaryInstruction, public NoTypePolicy::Data {
   uint32_t offset_;
   MWideningOp wideningOp_;
+  AliasSet aliases_;
 
   MWasmLoadField(MDefinition* obj, uint32_t offset, MIRType type,
-                 MWideningOp wideningOp)
+                 MWideningOp wideningOp, AliasSet aliases)
       : MUnaryInstruction(classOpcode, obj),
         offset_(offset),
-        wideningOp_(wideningOp) {
+        wideningOp_(wideningOp),
+        aliases_(aliases) {
     // "if you want to widen the value when it is loaded, the destination type
     // must be Int32".
     MOZ_ASSERT_IF(wideningOp != MWideningOp::None, type == MIRType::Int32);
+    MOZ_ASSERT(
+        aliases.flags() ==
+            AliasSet::Load(AliasSet::WasmStructOutlineDataPointer).flags() ||
+        aliases.flags() == AliasSet::Load(AliasSet::Any).flags());
     setResultType(type);
   }
 
@@ -10743,9 +10758,22 @@ class MWasmLoadField : public MUnaryInstruction, public NoTypePolicy::Data {
 
   uint32_t offset() const { return offset_; }
   MWideningOp wideningOp() const { return wideningOp_; }
-
+  AliasSet getAliasSet() const override { return aliases_; }
-  AliasSet getAliasSet() const override {
+  bool congruentTo(const MDefinition* ins) const override {
-    return AliasSet::Load(AliasSet::Any);
+    // In the limited case where this insn is used to read
+    // WasmStructObject::outlineData_ (the field itself, not what it points
+    // at), we allow commoning up to happen.  This is OK because
+    // WasmStructObject::outlineData_ is readonly for the life of the
+    // WasmStructObject.
+    if (!ins->isWasmLoadField()) {
+      return false;
+    }
+    const MWasmLoadField* other = ins->toWasmLoadField();
+    return ins->isWasmLoadField() && congruentIfOperandsEqual(ins) &&
+           offset() == other->offset() && wideningOp() == other->wideningOp() &&
+           getAliasSet().flags() == other->getAliasSet().flags() &&
+           getAliasSet().flags() ==
+               AliasSet::Load(AliasSet::WasmStructOutlineDataPointer).flags();
   }
 };
 
@@ -10762,13 +10790,21 @@ class MWasmLoadField : public MUnaryInstruction, public NoTypePolicy::Data {
 class MWasmLoadFieldKA : public MBinaryInstruction, public NoTypePolicy::Data {
   uint32_t offset_;
   MWideningOp wideningOp_;
+  AliasSet aliases_;
 
   MWasmLoadFieldKA(MDefinition* ka, MDefinition* obj, uint32_t offset,
-                   MIRType type, MWideningOp wideningOp)
+                   MIRType type, MWideningOp wideningOp, AliasSet aliases)
       : MBinaryInstruction(classOpcode, ka, obj),
         offset_(offset),
-        wideningOp_(wideningOp) {
+        wideningOp_(wideningOp),
+        aliases_(aliases) {
     MOZ_ASSERT_IF(wideningOp != MWideningOp::None, type == MIRType::Int32);
+    MOZ_ASSERT(
+        aliases.flags() ==
+            AliasSet::Load(AliasSet::WasmStructInlineDataArea).flags() ||
+        aliases.flags() ==
+            AliasSet::Load(AliasSet::WasmStructOutlineDataArea).flags() ||
+        aliases.flags() == AliasSet::Load(AliasSet::Any).flags());
     setResultType(type);
   }
 
@@ -10780,9 +10816,7 @@ class MWasmLoadFieldKA : public MBinaryInstruction, public NoTypePolicy::Data {
   uint32_t offset() const { return offset_; }
   MWideningOp wideningOp() const { return wideningOp_; }
 
-  AliasSet getAliasSet() const override {
+  AliasSet getAliasSet() const override { return aliases_; }
-    return AliasSet::Load(AliasSet::Any);
-  }
 };
 
 // Store a value to an object field at a fixed offset from a base pointer.
@@ -10795,17 +10829,26 @@ class MWasmStoreFieldKA : public MTernaryInstruction,
                           public NoTypePolicy::Data {
   uint32_t offset_;
   MNarrowingOp narrowingOp_;
+  AliasSet aliases_;
 
   MWasmStoreFieldKA(MDefinition* ka, MDefinition* obj, uint32_t offset,
-                    MDefinition* value, MNarrowingOp narrowingOp)
+                    MDefinition* value, MNarrowingOp narrowingOp,
+                    AliasSet aliases)
       : MTernaryInstruction(classOpcode, ka, obj, value),
         offset_(offset),
-        narrowingOp_(narrowingOp) {
+        narrowingOp_(narrowingOp),
+        aliases_(aliases) {
     MOZ_ASSERT(value->type() != MIRType::RefOrNull);
     // "if you want to narrow the value when it is stored, the source type
     // must be Int32".
     MOZ_ASSERT_IF(narrowingOp != MNarrowingOp::None,
                   value->type() == MIRType::Int32);
+    MOZ_ASSERT(
+        aliases.flags() ==
+            AliasSet::Store(AliasSet::WasmStructInlineDataArea).flags() ||
+        aliases.flags() ==
+            AliasSet::Store(AliasSet::WasmStructOutlineDataArea).flags() ||
+        aliases.flags() == AliasSet::Store(AliasSet::Any).flags());
   }
 
  public:
@@ -10816,9 +10859,7 @@ class MWasmStoreFieldKA : public MTernaryInstruction,
   uint32_t offset() const { return offset_; }
   MNarrowingOp narrowingOp() const { return narrowingOp_; }
 
-  AliasSet getAliasSet() const override {
+  AliasSet getAliasSet() const override { return aliases_; }
-    return AliasSet::Store(AliasSet::Any);
-  }
 };
 
 // Store a reference value to a location which (it is assumed) is within a
@@ -10829,11 +10870,20 @@ class MWasmStoreFieldKA : public MTernaryInstruction,
 // described for MWasmLoadFieldKA above.
 class MWasmStoreFieldRefKA : public MAryInstruction<4>,
                              public NoTypePolicy::Data {
+  AliasSet aliases_;
+
   MWasmStoreFieldRefKA(MDefinition* instance, MDefinition* ka,
-                       MDefinition* valueAddr, MDefinition* value)
+                       MDefinition* valueAddr, MDefinition* value,
-      : MAryInstruction<4>(classOpcode) {
+                       AliasSet aliases)
+      : MAryInstruction<4>(classOpcode), aliases_(aliases) {
     MOZ_ASSERT(valueAddr->type() == MIRType::Pointer);
     MOZ_ASSERT(value->type() == MIRType::RefOrNull);
+    MOZ_ASSERT(
+        aliases.flags() ==
+            AliasSet::Store(AliasSet::WasmStructInlineDataArea).flags() ||
+        aliases.flags() ==
+            AliasSet::Store(AliasSet::WasmStructOutlineDataArea).flags() ||
+        aliases.flags() == AliasSet::Store(AliasSet::Any).flags());
     initOperand(0, instance);
     initOperand(1, ka);
     initOperand(2, valueAddr);
@@ -10845,9 +10895,7 @@ class MWasmStoreFieldRefKA : public MAryInstruction<4>,
   TRIVIAL_NEW_WRAPPERS
   NAMED_OPERANDS((0, instance), (1, ka), (2, valueAddr), (3, value))
 
-  AliasSet getAliasSet() const override {
+  AliasSet getAliasSet() const override { return aliases_; }
-    return AliasSet::Store(AliasSet::Any);
-  }
 };
 
 #ifdef FUZZING_JS_FUZZILLI

js/src/wasm/WasmGcObject.h

@@ -215,7 +215,9 @@ class WasmStructObject : public WasmGcObject {
   static const JSClass class_;
 
   // Owned pointer to a malloc'd block containing out-of-line fields, or
-  // nullptr if none.
+  // nullptr if none.  Note that MIR alias analysis assumes this is readonly
+  // for the life of the object; do not change it once the object is created.
+  // See MWasmLoadObjectField::congruentTo.
   uint8_t* outlineData_;
 
   // The inline (wasm-struct-level) data fields.  This must be a multiple of

js/src/wasm/WasmIonCompile.cpp

@@ -3087,9 +3087,9 @@ class FunctionCompiler {
     const TagOffsetVector& offsets = tagType->argOffsets_;
 
     // Get the data pointer from the exception object
-    auto* data = MWasmLoadField::New(alloc(), exception,
+    auto* data = MWasmLoadField::New(
-                                     WasmExceptionObject::offsetOfData(),
+        alloc(), exception, WasmExceptionObject::offsetOfData(),
-                                     MIRType::Pointer, MWideningOp::None);
+        MIRType::Pointer, MWideningOp::None, AliasSet::Load(AliasSet::Any));
     if (!data) {
       return false;
     }
@@ -3102,9 +3102,9 @@ class FunctionCompiler {
 
     // Load each value from the data pointer
     for (size_t i = 0; i < params.length(); i++) {
-      auto* load =
+      auto* load = MWasmLoadFieldKA::New(
-          MWasmLoadFieldKA::New(alloc(), exception, data, offsets[i],
+          alloc(), exception, data, offsets[i], params[i].toMIRType(),
-                                params[i].toMIRType(), MWideningOp::None);
+          MWideningOp::None, AliasSet::Load(AliasSet::Any));
       if (!load || !values->append(load)) {
         return false;
       }
@@ -3213,9 +3213,9 @@ class FunctionCompiler {
     }
 
     // Load the data pointer from the object
-    auto* data = MWasmLoadField::New(alloc(), exception,
+    auto* data = MWasmLoadField::New(
-                                     WasmExceptionObject::offsetOfData(),
+        alloc(), exception, WasmExceptionObject::offsetOfData(),
-                                     MIRType::Pointer, MWideningOp::None);
+        MIRType::Pointer, MWideningOp::None, AliasSet::Load(AliasSet::Any));
     if (!data) {
       return false;
     }
@@ -3229,7 +3229,8 @@ class FunctionCompiler {
 
       if (!type.isRefRepr()) {
         auto* store = MWasmStoreFieldKA::New(alloc(), exception, data, offset,
-                                             argValues[i], MNarrowingOp::None);
+                                             argValues[i], MNarrowingOp::None,
+                                             AliasSet::Store(AliasSet::Any));
         if (!store) {
           return false;
         }
@@ -3245,9 +3246,9 @@ class FunctionCompiler {
       curBlock_->add(fieldAddr);
 
       // Load the previous value
-      auto* prevValue =
+      auto* prevValue = MWasmLoadFieldKA::New(
-          MWasmLoadFieldKA::New(alloc(), exception, data, offset,
+          alloc(), exception, data, offset, type.toMIRType(), MWideningOp::None,
-                                type.toMIRType(), MWideningOp::None);
+          AliasSet::Load(AliasSet::Any));
       if (!prevValue) {
         return false;
       }
@@ -3255,7 +3256,8 @@ class FunctionCompiler {
 
       // Store the new value
       auto* store = MWasmStoreFieldRefKA::New(
-          alloc(), instancePointer_, exception, fieldAddr, argValues[i]);
+          alloc(), instancePointer_, exception, fieldAddr, argValues[i],
+          AliasSet::Store(AliasSet::Any));
       if (!store) {
         return false;
       }
@@ -3433,9 +3435,10 @@ class FunctionCompiler {
     // accordingly.
     MDefinition* base;
     if (areaIsOutline) {
-      auto* load = MWasmLoadField::New(alloc(), structObject,
+      auto* load = MWasmLoadField::New(
-                                       WasmStructObject::offsetOfOutlineData(),
+          alloc(), structObject, WasmStructObject::offsetOfOutlineData(),
-                                       MIRType::Pointer, MWideningOp::None);
+          MIRType::Pointer, MWideningOp::None,
+          AliasSet::Load(AliasSet::WasmStructOutlineDataPointer));
       if (!load) {
         return false;
       }
@@ -3448,9 +3451,16 @@ class FunctionCompiler {
     // The transaction is to happen at `base + areaOffset`, so to speak.
     // After this point we must ignore `fieldOffset`.
 
+    // The alias set denoting the field's location, although lacking a
+    // Load-vs-Store indication at this point.
+    AliasSet::Flag fieldAliasSet = areaIsOutline
+                                       ? AliasSet::WasmStructOutlineDataArea
+                                       : AliasSet::WasmStructInlineDataArea;
+
     if (!fieldType.isRefRepr()) {
       auto* store = MWasmStoreFieldKA::New(alloc(), structObject, base,
-                                           areaOffset, value, fieldNarrowingOp);
+                                           areaOffset, value, fieldNarrowingOp,
+                                           AliasSet::Store(fieldAliasSet));
       if (!store) {
         return false;
       }
@@ -3477,17 +3487,18 @@ class FunctionCompiler {
     // results from struct.new, the old value is always zero.  So we should
     // synthesise a suitable zero constant rather than reading it from the
     // object.  See also bug 1799999.
-    auto* prevValue =
+    auto* prevValue = MWasmLoadFieldKA::New(
-        MWasmLoadFieldKA::New(alloc(), structObject, base, 0,
+        alloc(), structObject, base, 0, fieldType.toMIRType(),
-                              fieldType.toMIRType(), MWideningOp::None);
+        MWideningOp::None, AliasSet::Load(fieldAliasSet));
     if (!prevValue) {
       return false;
     }
     curBlock_->add(prevValue);
 
     // Store the new value
-    auto* store = MWasmStoreFieldRefKA::New(alloc(), instancePointer_,
+    auto* store =
-                                            structObject, base, value);
+        MWasmStoreFieldRefKA::New(alloc(), instancePointer_, structObject, base,
+                                  value, AliasSet::Store(fieldAliasSet));
     if (!store) {
       return false;
     }
@@ -3518,7 +3529,8 @@ class FunctionCompiler {
     if (areaIsOutline) {
       auto* loadOOLptr = MWasmLoadField::New(
           alloc(), structObject, WasmStructObject::offsetOfOutlineData(),
-          MIRType::Pointer, MWideningOp::None);
+          MIRType::Pointer, MWideningOp::None,
+          AliasSet::Load(AliasSet::WasmStructOutlineDataPointer));
       if (!loadOOLptr) {
         return nullptr;
       }
@@ -3531,11 +3543,18 @@ class FunctionCompiler {
     // The transaction is to happen at `base + areaOffset`, so to speak.
     // After this point we must ignore `fieldOffset`.
 
+    // The alias set denoting the field's location, although lacking a
+    // Load-vs-Store indication at this point.
+    AliasSet::Flag fieldAliasSet = areaIsOutline
+                                       ? AliasSet::WasmStructOutlineDataArea
+                                       : AliasSet::WasmStructInlineDataArea;
+
     MIRType mirType;
     MWideningOp mirWideningOp;
     fieldLoadInfoToMIR(fieldType, wideningOp, &mirType, &mirWideningOp);
-    auto* load = MWasmLoadFieldKA::New(alloc(), structObject, base, areaOffset,
+    auto* load =
-                                       mirType, mirWideningOp);
+        MWasmLoadFieldKA::New(alloc(), structObject, base, areaOffset, mirType,
+                              mirWideningOp, AliasSet::Load(fieldAliasSet));
     if (!load) {
       return nullptr;
     }