Bug 1546138 - Baldr: remove indirection from table.get return value (r=lth)
☠☠ backed out by e5562f9f81ca ☠ ☠
authorLuke Wagner <luke@mozilla.com>
Wed, 01 May 2019 15:13:43 -0500
changeset 532324 13e26dbd7cc7799690ea1cc4e4e1c35fb3de927b
parent 532323 edf39b4a6ec1260b6fc400bd55df352e5b0b2ebd
child 532325 0849b28c8e3db984273f97a8d6ea4f7284802f49
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth
bugs1546138
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1546138 - Baldr: remove indirection from table.get return value (r=lth) Differential Revision: https://phabricator.services.mozilla.com/D29596
js/src/jit/AliasAnalysis.cpp
js/src/jit/CodeGenerator.cpp
js/src/jit/Lowering.cpp
js/src/jit/MIR.h
js/src/jit/MacroAssembler.cpp
js/src/jit/shared/LIR-shared.h
js/src/wasm/WasmBaselineCompile.cpp
js/src/wasm/WasmBuiltins.cpp
js/src/wasm/WasmInstance.cpp
js/src/wasm/WasmIonCompile.cpp
js/src/wasm/WasmTable.cpp
js/src/wasm/WasmTable.h
js/src/wasm/WasmTypes.h
--- a/js/src/jit/AliasAnalysis.cpp
+++ b/js/src/jit/AliasAnalysis.cpp
@@ -174,17 +174,16 @@ static inline const MDefinition* GetObje
     case MDefinition::Opcode::WasmStore:
     case MDefinition::Opcode::WasmCompareExchangeHeap:
     case MDefinition::Opcode::WasmAtomicBinopHeap:
     case MDefinition::Opcode::WasmAtomicExchangeHeap:
     case MDefinition::Opcode::WasmLoadGlobalVar:
     case MDefinition::Opcode::WasmLoadGlobalCell:
     case MDefinition::Opcode::WasmStoreGlobalVar:
     case MDefinition::Opcode::WasmStoreGlobalCell:
-    case MDefinition::Opcode::WasmLoadRef:
     case MDefinition::Opcode::WasmStoreRef:
     case MDefinition::Opcode::ArrayJoin:
     case MDefinition::Opcode::ArraySlice:
       return nullptr;
     default:
 #ifdef DEBUG
       // Crash when the default aliasSet is overriden, but when not added in the
       // list above.
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -7526,20 +7526,16 @@ void CodeGenerator::visitWasmStoreSlot(L
   }
 }
 
 void CodeGenerator::visitWasmDerivedPointer(LWasmDerivedPointer* ins) {
   masm.movePtr(ToRegister(ins->base()), ToRegister(ins->output()));
   masm.addPtr(Imm32(int32_t(ins->offset())), ToRegister(ins->output()));
 }
 
-void CodeGenerator::visitWasmLoadRef(LWasmLoadRef* lir) {
-  masm.loadPtr(Address(ToRegister(lir->ptr()), 0), ToRegister(lir->output()));
-}
-
 void CodeGenerator::visitWasmStoreRef(LWasmStoreRef* ins) {
   Register tls = ToRegister(ins->tls());
   Register valueAddr = ToRegister(ins->valueAddr());
   Register value = ToRegister(ins->value());
   Register temp = ToRegister(ins->temp());
 
   Label skipPreBarrier;
   wasm::EmitWasmPreBarrierGuard(masm, tls, temp, valueAddr, &skipPreBarrier);
--- a/js/src/jit/Lowering.cpp
+++ b/js/src/jit/Lowering.cpp
@@ -4345,21 +4345,16 @@ void LIRGenerator::visitWasmStoreGlobalC
   }
 }
 
 void LIRGenerator::visitWasmDerivedPointer(MWasmDerivedPointer* ins) {
   LAllocation base = useRegisterAtStart(ins->base());
   define(new (alloc()) LWasmDerivedPointer(base), ins);
 }
 
-void LIRGenerator::visitWasmLoadRef(MWasmLoadRef* ins) {
-  define(new (alloc()) LWasmLoadRef(useRegisterAtStart(ins->getOperand(0))),
-         ins);
-}
-
 void LIRGenerator::visitWasmStoreRef(MWasmStoreRef* ins) {
   LAllocation tls = useRegister(ins->tls());
   LAllocation valueAddr = useFixed(ins->valueAddr(), PreBarrierReg);
   LAllocation value = useRegister(ins->value());
   add(new (alloc()) LWasmStoreRef(tls, valueAddr, value, temp()), ins);
 }
 
 void LIRGenerator::visitWasmParameter(MWasmParameter* ins) {
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -11770,41 +11770,16 @@ class MWasmDerivedPointer : public MUnar
 
   bool congruentTo(const MDefinition* ins) const override {
     return congruentIfOperandsEqual(ins);
   }
 
   ALLOW_CLONE(MWasmDerivedPointer)
 };
 
-class MWasmLoadRef : public MUnaryInstruction, public NoTypePolicy::Data {
-  AliasSet::Flag aliasSet_;
-
-  explicit MWasmLoadRef(MDefinition* valueAddr, AliasSet::Flag aliasSet,
-                        bool isMovable = true)
-      : MUnaryInstruction(classOpcode, valueAddr), aliasSet_(aliasSet) {
-    MOZ_ASSERT(valueAddr->type() == MIRType::Pointer);
-    setResultType(MIRType::RefOrNull);
-    if (isMovable) {
-      setMovable();
-    }
-  }
-
- public:
-  INSTRUCTION_HEADER(WasmLoadRef)
-  TRIVIAL_NEW_WRAPPERS
-
-  bool congruentTo(const MDefinition* ins) const override {
-    return congruentIfOperandsEqual(ins);
-  }
-  AliasSet getAliasSet() const override { return AliasSet::Load(aliasSet_); }
-
-  ALLOW_CLONE(MWasmLoadRef)
-};
-
 class MWasmStoreRef : public MAryInstruction<3>, public NoTypePolicy::Data {
   AliasSet::Flag aliasSet_;
 
   MWasmStoreRef(MDefinition* tls, MDefinition* valueAddr, MDefinition* value,
                 AliasSet::Flag aliasSet)
       : MAryInstruction<3>(classOpcode), aliasSet_(aliasSet) {
     MOZ_ASSERT(valueAddr->type() == MIRType::Pointer);
     MOZ_ASSERT(value->type() == MIRType::RefOrNull);
--- a/js/src/jit/MacroAssembler.cpp
+++ b/js/src/jit/MacroAssembler.cpp
@@ -3215,16 +3215,21 @@ CodeOffset MacroAssembler::wasmCallBuilt
       case wasm::FailureMode::Infallible:
         MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE();
       case wasm::FailureMode::FailOnNegI32:
         branchTest32(Assembler::NotSigned, ReturnReg, ReturnReg, &noTrap);
         break;
       case wasm::FailureMode::FailOnNullPtr:
         branchTestPtr(Assembler::NonZero, ReturnReg, ReturnReg, &noTrap);
         break;
+      case wasm::FailureMode::FailOnInvalidRef:
+        branchPtr(Assembler::NotEqual, ReturnReg,
+                  ImmWord(uintptr_t(wasm::AnyRef::invalid().forCompiledCode())),
+                  &noTrap);
+        break;
     }
     wasmTrap(wasm::Trap::ThrowReported,
              wasm::BytecodeOffset(desc.lineOrBytecode()));
     bind(&noTrap);
   }
 
   return ret;
 }
--- a/js/src/jit/shared/LIR-shared.h
+++ b/js/src/jit/shared/LIR-shared.h
@@ -6677,27 +6677,16 @@ class LWasmDerivedPointer : public LInst
   explicit LWasmDerivedPointer(const LAllocation& base)
       : LInstructionHelper(classOpcode) {
     setOperand(0, base);
   }
   const LAllocation* base() { return getOperand(0); }
   size_t offset() { return mirRaw()->toWasmDerivedPointer()->offset(); }
 };
 
-class LWasmLoadRef : public LInstructionHelper<1, 1, 0> {
- public:
-  LIR_HEADER(WasmLoadRef);
-  explicit LWasmLoadRef(const LAllocation& ptr)
-      : LInstructionHelper(classOpcode) {
-    setOperand(0, ptr);
-  }
-  MWasmLoadRef* mir() const { return mirRaw()->toWasmLoadRef(); }
-  const LAllocation* ptr() { return getOperand(0); }
-};
-
 class LWasmStoreRef : public LInstructionHelper<0, 3, 1> {
  public:
   LIR_HEADER(WasmStoreRef);
   LWasmStoreRef(const LAllocation& tls, const LAllocation& valueAddr,
                 const LAllocation& value, const LDefinition& temp)
       : LInstructionHelper(classOpcode) {
     setOperand(0, tls);
     setOperand(1, valueAddr);
--- a/js/src/wasm/WasmBaselineCompile.cpp
+++ b/js/src/wasm/WasmBaselineCompile.cpp
@@ -10328,28 +10328,23 @@ bool BaseCompiler::emitTableGet() {
   Nothing index;
   uint32_t tableIndex;
   if (!iter_.readTableGet(&tableIndex, &index)) {
     return false;
   }
   if (deadCode_) {
     return true;
   }
-  // get(index:u32, table:u32) -> void*
-  //
-  // Returns a pointer to a nonmoveable memory location that holds the anyref
-  // value.
+  // get(index:u32, table:u32) -> uintptr_t(AnyRef)
   pushI32(tableIndex);
   if (!emitInstanceCall(lineOrBytecode, SASigTableGet,
                         /*pushReturnedValue=*/false)) {
     return false;
   }
 
-  masm.loadPtr(Address(ReturnReg, 0), ReturnReg);
-
   // Push the resulting anyref back on the eval stack.  NOTE: needRef() must
   // not kill the value in the register.
   RegPtr r = RegPtr(ReturnReg);
   needRef(r);
   pushRef(r);
 
   return true;
 }
--- a/js/src/wasm/WasmBuiltins.cpp
+++ b/js/src/wasm/WasmBuiltins.cpp
@@ -54,16 +54,17 @@ static const unsigned BUILTIN_THUNK_LIFO
 #define _I64 MIRType::Int64
 #define _PTR MIRType::Pointer
 #define _RoN MIRType::RefOrNull
 #define _VOID MIRType::None
 #define _END MIRType::None
 #define _Infallible FailureMode::Infallible
 #define _FailOnNegI32 FailureMode::FailOnNegI32
 #define _FailOnNullPtr FailureMode::FailOnNullPtr
+#define _FailOnInvalidRef FailureMode::FailOnInvalidRef
 
 namespace js {
 namespace wasm {
 
 const SymbolicAddressSignature SASigSinD = {
     SymbolicAddress::SinD, _F64, _Infallible, 1, {_F64, _END}};
 const SymbolicAddressSignature SASigCosD = {
     SymbolicAddress::CosD, _F64, _Infallible, 1, {_F64, _END}};
@@ -143,18 +144,18 @@ const SymbolicAddressSignature SASigElem
     SymbolicAddress::ElemDrop, _VOID, _FailOnNegI32, 2, {_PTR, _I32, _END}};
 const SymbolicAddressSignature SASigTableFill = {
     SymbolicAddress::TableFill,
     _VOID,
     _FailOnNegI32,
     5,
     {_PTR, _I32, _RoN, _I32, _I32, _END}};
 const SymbolicAddressSignature SASigTableGet = {SymbolicAddress::TableGet,
-                                                _PTR,
-                                                _FailOnNullPtr,
+                                                _RoN,
+                                                _FailOnInvalidRef,
                                                 3,
                                                 {_PTR, _I32, _I32, _END}};
 const SymbolicAddressSignature SASigTableGrow = {
     SymbolicAddress::TableGrow,
     _I32,
     _Infallible,
     4,
     {_PTR, _RoN, _I32, _I32, _END}};
--- a/js/src/wasm/WasmInstance.cpp
+++ b/js/src/wasm/WasmInstance.cpp
@@ -301,16 +301,17 @@ Instance::callImport_anyref(Instance* in
   RootedValue rval(cx);
   if (!instance->callImport(cx, funcImportIndex, argc, argv, &rval)) {
     return false;
   }
   RootedAnyRef result(cx, AnyRef::null());
   if (!BoxAnyRef(cx, rval, &result)) {
     return false;
   }
+  static_assert(sizeof(argv[0]) >= sizeof(void*), "fits");
   *(void**)argv = result.get().forCompiledCode();
   return true;
 }
 
 /* static */ int32_t /* 0 to signal trap; 1 to signal OK */
 Instance::callImport_funcref(Instance* instance, int32_t funcImportIndex,
                              int32_t argc, uint64_t* argv) {
   JSContext* cx = TlsContext.get();
@@ -929,37 +930,27 @@ void Instance::initElems(uint32_t tableI
   }
 
   JSContext* cx = TlsContext.get();
   JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                             JSMSG_WASM_TABLE_OUT_OF_BOUNDS);
   return -1;
 }
 
-// The return convention for tableGet() is awkward but avoids a situation where
-// Ion code has to hold a value that may or may not be a pointer to GC'd
-// storage, or where Ion has to pass in a pointer to storage where a return
-// value can be written.
-//
-// Note carefully that the pointer that is returned may not be valid past
-// operations that change the size of the table or cause GC work; it is strictly
-// to be used to retrieve the return value.
-
 /* static */ void* Instance::tableGet(Instance* instance, uint32_t index,
                                       uint32_t tableIndex) {
-  MOZ_ASSERT(SASigTableGet.failureMode == FailureMode::FailOnNullPtr);
-
+  MOZ_ASSERT(SASigTableGet.failureMode == FailureMode::FailOnInvalidRef);
   const Table& table = *instance->tables()[tableIndex];
   MOZ_RELEASE_ASSERT(table.kind() == TableKind::AnyRef);
   if (index >= table.length()) {
     JS_ReportErrorNumberASCII(TlsContext.get(), GetErrorMessage, nullptr,
                               JSMSG_WASM_TABLE_OUT_OF_BOUNDS);
-    return nullptr;
+    return AnyRef::invalid().forCompiledCode();
   }
-  return const_cast<void*>(table.getShortlivedAnyRefLocForCompiledCode(index));
+  return table.getAnyRef(index).forCompiledCode();
 }
 
 /* static */ uint32_t Instance::tableGrow(Instance* instance, void* initValue,
                                           uint32_t delta, uint32_t tableIndex) {
   MOZ_ASSERT(SASigTableGrow.failureMode == FailureMode::Infallible);
 
   RootedAnyRef obj(TlsContext.get(), AnyRef::fromCompiledCode(initValue));
   Table& table = *instance->tables()[tableIndex];
--- a/js/src/wasm/WasmIonCompile.cpp
+++ b/js/src/wasm/WasmIonCompile.cpp
@@ -676,26 +676,16 @@ class FunctionCompiler {
     }
     auto* ins =
         MWasmAddOffset::New(alloc(), base, access->offset(), bytecodeOffset());
     curBlock_->add(ins);
     access->clearOffset();
     return ins;
   }
 
-  MDefinition* derefTableElementPointer(MDefinition* base) {
-    // Table element storage may be moved by GC operations, so reads from that
-    // storage are not movable.
-    MWasmLoadRef* load =
-        MWasmLoadRef::New(alloc(), base, AliasSet::WasmTableElement,
-                          /*isMovable=*/false);
-    curBlock_->add(load);
-    return load;
-  }
-
   MDefinition* load(MDefinition* base, MemoryAccessDesc* access,
                     ValType result) {
     if (inDeadCode()) {
       return nullptr;
     }
 
     MWasmLoadTls* memoryBase = maybeLoadMemoryBase();
     MInstruction* load = nullptr;
@@ -3129,23 +3119,18 @@ static bool EmitTableGet(FunctionCompile
   }
 
   if (!f.finishCall(&args)) {
     return false;
   }
 
   // The return value here is either null, denoting an error, or a short-lived
   // pointer to a location containing a possibly-null ref.
-  MDefinition* result;
-  if (!f.builtinInstanceMethodCall(callee, lineOrBytecode, args, &result)) {
-    return false;
-  }
-
-  MDefinition* ret = f.derefTableElementPointer(result);
-  if (!ret) {
+  MDefinition* ret;
+  if (!f.builtinInstanceMethodCall(callee, lineOrBytecode, args, &ret)) {
     return false;
   }
 
   f.iter().setResult(ret);
   return true;
 }
 
 static bool EmitTableGrow(FunctionCompiler& f) {
--- a/js/src/wasm/WasmTable.cpp
+++ b/js/src/wasm/WasmTable.cpp
@@ -143,22 +143,16 @@ const FunctionTableElem& Table::getFuncR
 AnyRef Table::getAnyRef(uint32_t index) const {
   MOZ_ASSERT(!isFunction());
   // TODO/AnyRef-boxing: With boxed immediates and strings, the write barrier
   // is going to have to be more complicated.
   ASSERT_ANYREF_IS_JSOBJECT;
   return AnyRef::fromJSObject(objects_[index]);
 }
 
-const void* Table::getShortlivedAnyRefLocForCompiledCode(uint32_t index) const {
-  MOZ_ASSERT(!isFunction());
-  return const_cast<HeapPtr<JSObject*>&>(objects_[index])
-      .unsafeUnbarrieredForTracing();
-}
-
 void Table::setFuncRef(uint32_t index, void* code, const Instance* instance) {
   MOZ_ASSERT(isFunction());
 
   FunctionTableElem& elem = functions_[index];
   if (elem.tls) {
     JSObject::writeBarrierPre(elem.tls->instance->objectUnbarriered());
   }
 
--- a/js/src/wasm/WasmTable.h
+++ b/js/src/wasm/WasmTable.h
@@ -81,17 +81,16 @@ class Table : public ShareableBase<Table
 
   // get/setFuncRef is allowed only on table-of-funcref.
   // get/setAnyRef is allowed only on table-of-anyref.
   // setNull is allowed on either.
   const FunctionTableElem& getFuncRef(uint32_t index) const;
   void setFuncRef(uint32_t index, void* code, const Instance* instance);
 
   AnyRef getAnyRef(uint32_t index) const;
-  const void* getShortlivedAnyRefLocForCompiledCode(uint32_t index) const;
   void setAnyRef(uint32_t index, AnyRef);
 
   void setNull(uint32_t index);
 
   // Copy entry from |srcTable| at |srcIndex| to this table at |dstIndex|.
   // Used by table.copy.
   void copy(const Table& srcTable, uint32_t dstIndex, uint32_t srcIndex);
 
--- a/js/src/wasm/WasmTypes.h
+++ b/js/src/wasm/WasmTypes.h
@@ -563,21 +563,26 @@ static inline const char* ToCString(ValT
 // For version 0, we simply equate AnyRef and JSObject* (this means that there
 // are technically no tags at all yet).  We use a simple boxing scheme that
 // wraps a JS value that is not already JSObject in a distinguishable JSObject
 // that holds the value, see WasmTypes.cpp for details.
 
 class AnyRef {
   JSObject* value_;
 
+  explicit AnyRef() : value_((JSObject*)-1) {}
   explicit AnyRef(JSObject* p) : value_(p) {
     MOZ_ASSERT(((uintptr_t)p & 0x03) == 0);
   }
 
  public:
+  // An invalid AnyRef cannot arise naturally from wasm and so can be used as
+  // a sentinel value to indicate failure from an AnyRef-returning function.
+  static AnyRef invalid() { return AnyRef(); }
+
   // Given a void* that comes from compiled wasm code, turn it into AnyRef.
   static AnyRef fromCompiledCode(void* p) { return AnyRef((JSObject*)p); }
 
   // Given a JSObject* that comes from JS, turn it into AnyRef.
   static AnyRef fromJSObject(JSObject* p) { return AnyRef(p); }
 
   // Generate an AnyRef null pointer.
   static AnyRef null() { return AnyRef(nullptr); }
@@ -1900,17 +1905,22 @@ enum class SymbolicAddress {
   Limit
 };
 
 // The FailureMode indicates whether, immediately after a call to a builtin
 // returns, the return value should be checked against an error condition
 // (and if so, which one) which signals that the C++ calle has already
 // reported an error and thus wasm needs to wasmTrap(Trap::ThrowReported).
 
-enum class FailureMode : uint8_t { Infallible, FailOnNegI32, FailOnNullPtr };
+enum class FailureMode : uint8_t {
+  Infallible,
+  FailOnNegI32,
+  FailOnNullPtr,
+  FailOnInvalidRef
+};
 
 // SymbolicAddressSignature carries type information for a function referred
 // to by a SymbolicAddress.  In order that |argTypes| can be written out as a
 // static initialiser, it has to have fixed length.  At present
 // SymbolicAddressType is used to describe functions with at most 6 arguments,
 // so |argTypes| has 7 entries in order to allow the last value to be
 // MIRType::None, in the hope of catching any accidental overruns of the
 // defined section of the array.