Bug 1574415 - Part 3: Add AutoScratchFloatRegister. r=jandem
authorAndré Bargull <andre.bargull@gmail.com>
Mon, 07 Oct 2019 11:56:44 +0000
changeset 496544 3f73e398ca61d24ddb9f8de379e1c077452ff9b1
parent 496543 6067d1828df8f7ca054c4a57430e466e1987aa75
child 496545 ef1f7213e654816da44e0963a27adf1510e1ea03
push id36661
push userccoroiu@mozilla.com
push dateMon, 07 Oct 2019 21:50:01 +0000
treeherdermozilla-central@2b4c8b7a255c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1574415
milestone71.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 1574415 - Part 3: Add AutoScratchFloatRegister. r=jandem Add a RAII class to spill and restore `Float0` when used as a scratch register when generating Ion CacheIR assembly. It's still possible to generate incorrect code which doesn't properly restore `Float0`, for example through jump instructions, but the RAII class should at least prevent bugs like in `CacheIRCompiler::emitLoadDoubleTruthyResult` where `Float0` wasn't restored for the truthy case. Differential Revision: https://phabricator.services.mozilla.com/D47751
js/src/jit/CacheIRCompiler.cpp
js/src/jit/CacheIRCompiler.h
--- a/js/src/jit/CacheIRCompiler.cpp
+++ b/js/src/jit/CacheIRCompiler.cpp
@@ -1548,34 +1548,23 @@ bool CacheIRCompiler::emitGuardToInt32In
   masm.branchTestInt32(Assembler::NotEqual, input, &notInt32);
   masm.unboxInt32(input, output);
   masm.jump(&done);
 
   masm.bind(&notInt32);
 
   masm.branchTestDouble(Assembler::NotEqual, input, failure->label());
 
-  // If we're compiling a Baseline IC, FloatReg0 is always available.
-  Label failurePopReg;
-  if (mode_ != Mode::Baseline) {
-    masm.push(FloatReg0);
-  }
-
-  masm.unboxDouble(input, FloatReg0);
-  // ToPropertyKey(-0.0) is "0", so we can truncate -0.0 to 0 here.
-  masm.convertDoubleToInt32(
-      FloatReg0, output,
-      (mode_ == Mode::Baseline) ? failure->label() : &failurePopReg, false);
-  if (mode_ != Mode::Baseline) {
-    masm.pop(FloatReg0);
-    masm.jump(&done);
-
-    masm.bind(&failurePopReg);
-    masm.pop(FloatReg0);
-    masm.jump(failure->label());
+  {
+    AutoScratchFloatRegister floatReg(this, failure);
+
+    masm.unboxDouble(input, floatReg);
+
+    // ToPropertyKey(-0.0) is "0", so we can truncate -0.0 to 0 here.
+    masm.convertDoubleToInt32(floatReg, output, floatReg.failure(), false);
   }
 
   masm.bind(&done);
   return true;
 }
 
 bool CacheIRCompiler::emitGuardType() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
@@ -2505,80 +2494,48 @@ bool CacheIRCompiler::emitDoubleNegation
   AutoOutputRegister output(*this);
   ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
 
   FailurePath* failure;
   if (!addFailurePath(&failure)) {
     return false;
   }
 
-  // If we're compiling a Baseline IC, FloatReg0 is always available.
-  Label failurePopReg, done;
-  if (mode_ != Mode::Baseline) {
-    masm.push(FloatReg0);
-  }
-
-  masm.ensureDouble(
-      val, FloatReg0,
-      (mode_ != Mode::Baseline) ? &failurePopReg : failure->label());
-  masm.negateDouble(FloatReg0);
-  masm.boxDouble(FloatReg0, output.valueReg(), FloatReg0);
-
-  if (mode_ != Mode::Baseline) {
-    masm.pop(FloatReg0);
-    masm.jump(&done);
-
-    masm.bind(&failurePopReg);
-    masm.pop(FloatReg0);
-    masm.jump(failure->label());
-  }
-
-  masm.bind(&done);
+  AutoScratchFloatRegister floatReg(this, failure);
+
+  masm.ensureDouble(val, floatReg, floatReg.failure());
+  masm.negateDouble(floatReg);
+  masm.boxDouble(floatReg, output.valueReg(), floatReg);
+
   return true;
 }
 
 bool CacheIRCompiler::emitDoubleIncDecResult(bool isInc) {
   AutoOutputRegister output(*this);
   ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
 
   FailurePath* failure;
   if (!addFailurePath(&failure)) {
     return false;
   }
 
-  // If we're compiling a Baseline IC, FloatReg0 is always available.
-  Label failurePopReg, done;
-  if (mode_ != Mode::Baseline) {
-    masm.push(FloatReg0);
-  }
-
-  masm.ensureDouble(
-      val, FloatReg0,
-      (mode_ != Mode::Baseline) ? &failurePopReg : failure->label());
+  AutoScratchFloatRegister floatReg(this, failure);
+
+  masm.ensureDouble(val, floatReg, floatReg.failure());
   {
     ScratchDoubleScope fpscratch(masm);
     masm.loadConstantDouble(1.0, fpscratch);
     if (isInc) {
-      masm.addDouble(fpscratch, FloatReg0);
+      masm.addDouble(fpscratch, floatReg);
     } else {
-      masm.subDouble(fpscratch, FloatReg0);
+      masm.subDouble(fpscratch, floatReg);
     }
   }
-  masm.boxDouble(FloatReg0, output.valueReg(), FloatReg0);
-
-  if (mode_ != Mode::Baseline) {
-    masm.pop(FloatReg0);
-    masm.jump(&done);
-
-    masm.bind(&failurePopReg);
-    masm.pop(FloatReg0);
-    masm.jump(failure->label());
-  }
-
-  masm.bind(&done);
+  masm.boxDouble(floatReg, output.valueReg(), floatReg);
+
   return true;
 }
 
 bool CacheIRCompiler::emitDoubleIncResult() {
   return emitDoubleIncDecResult(true);
 }
 
 bool CacheIRCompiler::emitDoubleDecResult() {
@@ -2588,47 +2545,45 @@ bool CacheIRCompiler::emitDoubleDecResul
 bool CacheIRCompiler::emitTruncateDoubleToUInt32() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
   Register res = allocator.defineRegister(masm, reader.int32OperandId());
 
   Label int32, done;
   masm.branchTestInt32(Assembler::Equal, val, &int32);
 
-  Label doneTruncate, truncateABICall;
-  if (mode_ != Mode::Baseline) {
-    masm.push(FloatReg0);
+  {
+    Label doneTruncate, truncateABICall;
+
+    AutoScratchFloatRegister floatReg(this);
+
+    masm.unboxDouble(val, floatReg);
+    masm.branchTruncateDoubleMaybeModUint32(floatReg, res, &truncateABICall);
+    masm.jump(&doneTruncate);
+
+    masm.bind(&truncateABICall);
+    LiveRegisterSet save(GeneralRegisterSet::Volatile(),
+                         liveVolatileFloatRegs());
+    save.takeUnchecked(floatReg);
+    // Bug 1451976
+    save.takeUnchecked(floatReg.get().asSingle());
+    masm.PushRegsInMask(save);
+
+    masm.setupUnalignedABICall(res);
+    masm.passABIArg(floatReg, MoveOp::DOUBLE);
+    masm.callWithABI(BitwiseCast<void*, int32_t (*)(double)>(JS::ToInt32),
+                     MoveOp::GENERAL, CheckUnsafeCallWithABI::DontCheckOther);
+    masm.storeCallInt32Result(res);
+
+    LiveRegisterSet ignore;
+    ignore.add(res);
+    masm.PopRegsInMaskIgnore(save, ignore);
+
+    masm.bind(&doneTruncate);
   }
-
-  masm.unboxDouble(val, FloatReg0);
-  masm.branchTruncateDoubleMaybeModUint32(FloatReg0, res, &truncateABICall);
-  masm.jump(&doneTruncate);
-
-  masm.bind(&truncateABICall);
-  LiveRegisterSet save(GeneralRegisterSet::Volatile(), liveVolatileFloatRegs());
-  save.takeUnchecked(FloatReg0);
-  // Bug 1451976
-  save.takeUnchecked(FloatReg0.asSingle());
-  masm.PushRegsInMask(save);
-
-  masm.setupUnalignedABICall(res);
-  masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
-  masm.callWithABI(BitwiseCast<void*, int32_t (*)(double)>(JS::ToInt32),
-                   MoveOp::GENERAL, CheckUnsafeCallWithABI::DontCheckOther);
-  masm.storeCallInt32Result(res);
-
-  LiveRegisterSet ignore;
-  ignore.add(res);
-  masm.PopRegsInMaskIgnore(save, ignore);
-
-  masm.bind(&doneTruncate);
-  if (mode_ != Mode::Baseline) {
-    masm.pop(FloatReg0);
-  }
-
   masm.jump(&done);
   masm.bind(&int32);
 
   masm.unboxInt32(val, res);
 
   masm.bind(&done);
   return true;
 }
@@ -3491,35 +3446,29 @@ bool CacheIRCompiler::emitLoadStringTrut
   return true;
 }
 
 bool CacheIRCompiler::emitLoadDoubleTruthyResult() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   AutoOutputRegister output(*this);
   ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
 
-  Label ifFalse, done, failurePopReg;
-
-  // If we're compiling a Baseline IC, FloatReg0 is always available.
-  if (mode_ != Mode::Baseline) {
-    masm.push(FloatReg0);
-  }
-
-  masm.unboxDouble(val, FloatReg0);
-
-  masm.branchTestDoubleTruthy(false, FloatReg0, &ifFalse);
+  AutoScratchFloatRegister floatReg(this);
+
+  Label ifFalse, done;
+
+  masm.unboxDouble(val, floatReg);
+
+  masm.branchTestDoubleTruthy(false, floatReg, &ifFalse);
   masm.moveValue(BooleanValue(true), output.valueReg());
   masm.jump(&done);
 
   masm.bind(&ifFalse);
   masm.moveValue(BooleanValue(false), output.valueReg());
 
-  if (mode_ != Mode::Baseline) {
-    masm.pop(FloatReg0);
-  }
   masm.bind(&done);
   return true;
 }
 
 bool CacheIRCompiler::emitLoadObjectTruthyResult() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   AutoOutputRegister output(*this);
   Register obj = allocator.useRegister(masm, reader.objOperandId());
@@ -4562,8 +4511,51 @@ AutoCallVM::~AutoCallVM() {
     if (output_.isSome()) {
       masm_.storeCallResultValue(output_.ref());
     }
     return;
   }
   MOZ_ASSERT(compiler_->mode_ == CacheIRCompiler::Mode::Baseline);
   stubFrame_->leave(masm_);
 }
+
+AutoScratchFloatRegister::AutoScratchFloatRegister(CacheIRCompiler* compiler,
+                                                   FailurePath* failure)
+    : compiler_(compiler), failure_(failure) {
+  // If we're compiling a Baseline IC, FloatReg0 is always available.
+  if (!compiler_->isBaseline()) {
+    MacroAssembler& masm = compiler_->masm;
+    masm.push(FloatReg0);
+  }
+
+  if (failure_) {
+    failure_->setHasAutoScratchFloatRegister();
+  }
+}
+
+AutoScratchFloatRegister::~AutoScratchFloatRegister() {
+  if (failure_) {
+    failure_->clearHasAutoScratchFloatRegister();
+  }
+
+  if (!compiler_->isBaseline()) {
+    MacroAssembler& masm = compiler_->masm;
+    masm.pop(FloatReg0);
+
+    if (failure_) {
+      Label done;
+      masm.jump(&done);
+      masm.bind(&failurePopReg_);
+      masm.pop(FloatReg0);
+      masm.jump(failure_->label());
+      masm.bind(&done);
+    }
+  }
+}
+
+Label* AutoScratchFloatRegister::failure() {
+  MOZ_ASSERT(failure_);
+
+  if (!compiler_->isBaseline()) {
+    return &failurePopReg_;
+  }
+  return failure_->labelUnchecked();
+}
--- a/js/src/jit/CacheIRCompiler.h
+++ b/js/src/jit/CacheIRCompiler.h
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef jit_CacheIRCompiler_h
 #define jit_CacheIRCompiler_h
 
 #include "mozilla/Maybe.h"
 
 #include "jit/CacheIR.h"
+#include "jit/SharedICRegisters.h"
 
 namespace js {
 namespace jit {
 
 class BaselineCacheIRCompiler;
 class IonCacheIRCompiler;
 
 // The ops below are defined in CacheIRCompiler and codegen is shared between
@@ -653,27 +654,36 @@ class MOZ_RAII AutoScratchRegister {
 // The FailurePath class stores everything we need to generate a failure path
 // at the end of the IC code. The failure path restores the input registers, if
 // needed, and jumps to the next stub.
 class FailurePath {
   Vector<OperandLocation, 4, SystemAllocPolicy> inputs_;
   SpilledRegisterVector spilledRegs_;
   NonAssertingLabel label_;
   uint32_t stackPushed_;
+#ifdef DEBUG
+  // Flag to ensure FailurePath::label() isn't taken while there's a scratch
+  // float register which still needs to be restored.
+  bool hasAutoScratchFloatRegister_ = false;
+#endif
 
  public:
   FailurePath() = default;
 
   FailurePath(FailurePath&& other)
       : inputs_(std::move(other.inputs_)),
         spilledRegs_(std::move(other.spilledRegs_)),
         label_(other.label_),
         stackPushed_(other.stackPushed_) {}
 
-  Label* label() { return &label_; }
+  Label* labelUnchecked() { return &label_; }
+  Label* label() {
+    MOZ_ASSERT(!hasAutoScratchFloatRegister_);
+    return labelUnchecked();
+  }
 
   void setStackPushed(uint32_t i) { stackPushed_ = i; }
   uint32_t stackPushed() const { return stackPushed_; }
 
   MOZ_MUST_USE bool appendInput(const OperandLocation& loc) {
     return inputs_.append(loc);
   }
   OperandLocation input(size_t i) const { return inputs_[i]; }
@@ -683,16 +693,30 @@ class FailurePath {
   MOZ_MUST_USE bool setSpilledRegs(const SpilledRegisterVector& regs) {
     MOZ_ASSERT(spilledRegs_.empty());
     return spilledRegs_.appendAll(regs);
   }
 
   // If canShareFailurePath(other) returns true, the same machine code will
   // be emitted for two failure paths, so we can share them.
   bool canShareFailurePath(const FailurePath& other) const;
+
+  void setHasAutoScratchFloatRegister() {
+#ifdef DEBUG
+    MOZ_ASSERT(!hasAutoScratchFloatRegister_);
+    hasAutoScratchFloatRegister_ = true;
+#endif
+  }
+
+  void clearHasAutoScratchFloatRegister() {
+#ifdef DEBUG
+    MOZ_ASSERT(hasAutoScratchFloatRegister_);
+    hasAutoScratchFloatRegister_ = false;
+#endif
+  }
 };
 
 /**
  * Wrap an offset so that a call can decide to embed a constant
  * or load from the stub data.
  */
 class StubFieldOffset {
  private:
@@ -711,16 +735,17 @@ class AutoOutputRegister;
 
 // Base class for BaselineCacheIRCompiler and IonCacheIRCompiler.
 class MOZ_RAII CacheIRCompiler {
  protected:
   friend class AutoOutputRegister;
   friend class AutoStubFrame;
   friend class AutoSaveLiveRegisters;
   friend class AutoCallVM;
+  friend class AutoScratchFloatRegister;
 
   enum class Mode { Baseline, Ion };
 
   bool preparedForVMCall_;
 
   bool isBaseline();
   bool isIon();
   BaselineCacheIRCompiler* asBaseline();
@@ -1071,16 +1096,45 @@ class MOZ_RAII AutoCallVM {
   AutoCallVM(MacroAssembler& masm, CacheIRCompiler* compiler,
              CacheRegisterAllocator& allocator);
 
   void prepare();
 
   ~AutoCallVM();
 };
 
+// RAII class to allocate FloatReg0 as a scratch register and release it when
+// we're done with it. The previous contents of FloatReg0 may be spilled on the
+// stack and, if necessary, are restored when the destructor runs.
+//
+// When FailurePath is passed to the constructor, FailurePath::label() must not
+// be used during the life time of the AutoScratchFloatRegister. Instead use
+// AutoScratchFloatRegister::failure().
+class MOZ_RAII AutoScratchFloatRegister {
+  Label failurePopReg_{};
+  CacheIRCompiler* compiler_;
+  FailurePath* failure_;
+
+  AutoScratchFloatRegister(const AutoScratchFloatRegister&) = delete;
+  void operator=(const AutoScratchFloatRegister&) = delete;
+
+ public:
+  explicit AutoScratchFloatRegister(CacheIRCompiler* compiler)
+      : AutoScratchFloatRegister(compiler, nullptr) {}
+
+  AutoScratchFloatRegister(CacheIRCompiler* compiler, FailurePath* failure);
+
+  ~AutoScratchFloatRegister();
+
+  Label* failure();
+
+  FloatRegister get() const { return FloatReg0; }
+  operator FloatRegister() const { return FloatReg0; }
+};
+
 // See the 'Sharing Baseline stub code' comment in CacheIR.h for a description
 // of this class.
 class CacheIRStubInfo {
   // These fields don't require 8 bits, but GCC complains if these fields are
   // smaller than the size of the enums.
   CacheKind kind_ : 8;
   ICStubEngine engine_ : 8;
   bool makesGCCalls_ : 1;