Bug 1683535: Check hadEagerTruncationBailout when truncating phis r=jandem
authorIain Ireland <iireland@mozilla.com>
Tue, 05 Jan 2021 13:53:42 +0000
changeset 562077 114f361d728d53bfdc3e337e6abe78e917904529
parent 562076 7944733c5201f9f389b01a88fc1e9851e964493a
child 562078 fdbcd1cd62e215d916777f5ff305fbdb6c4da9ef
push id38080
push userncsoregi@mozilla.com
push dateWed, 06 Jan 2021 03:51:26 +0000
treeherdermozilla-central@1e323e0a130c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1683535
milestone86.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 1683535: Check hadEagerTruncationBailout when truncating phis r=jandem There were two bugs here: 1. We weren't checking hadEagerTruncationBailout before eagerly truncating phis. 2. MDiv::operandTruncateKind and MMod::operandTruncateKind can return TruncateAfterBailouts even if ComputeTruncateKind returns a less restrictive kind. We therefore have to check the operands too. Depends on D100750 Differential Revision: https://phabricator.services.mozilla.com/D100751
js/src/jit-test/tests/warp/bug1683535-1.js
js/src/jit-test/tests/warp/bug1683535-2.js
js/src/jit/MIR.h
js/src/jit/RangeAnalysis.cpp
js/src/jit/RangeAnalysis.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/warp/bug1683535-1.js
@@ -0,0 +1,6 @@
+function f(x, y) {
+    (Math.log() ? 0 : Math.abs(~y)) ^ x ? x : x;
+}
+for (let i = 0; i < 52; i++) {
+    f(0, -2147483649);
+}
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/warp/bug1683535-2.js
@@ -0,0 +1,10 @@
+function testMathyFunction(f, inputs) {
+  var results = [];
+  for (var j = 0; j < inputs.length; ++j) 
+    for (var k = 0; k < inputs.length; ++k) 
+      results.push(f(inputs[j], inputs[k]));
+}
+mathy0 = (function(x, y) {
+  return (Math.clz32((x <= x) >>> y) >> (~(0x080000000 >>> 0))) % Math.acos(~(2 ** 53)) >>> 0
+});
+testMathyFunction(mathy0, [1, 42, 0 / 0, 1 / 0, -Number.MIN_SAFE_INTEGER, -(2 ** 53), (2 ** 53), 1.7976931348623157e308]);
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -449,17 +449,17 @@ class StoreDependency : public TempObjec
     }
     return true;
   }
 
   MStoreVector& get() { return all_; }
 };
 
 // When a floating-point value is used by nodes which would prefer to
-// recieve integer inputs, we may be able to help by computing our result
+// receive integer inputs, we may be able to help by computing our result
 // into an integer directly.
 //
 // A value can be truncated in 4 differents ways:
 //   1. Ignore Infinities (x / 0 --> 0).
 //   2. Ignore overflow (INT_MIN / -1 == (INT_MAX + 1) --> INT_MIN)
 //   3. Ignore negative zeros. (-0 --> 0)
 //   4. Ignore remainder. (3 / 4 --> 0)
 //
--- a/js/src/jit/RangeAnalysis.cpp
+++ b/js/src/jit/RangeAnalysis.cpp
@@ -3080,16 +3080,44 @@ static void AdjustTruncatedInputs(TempAl
   }
 
   if (truncated->isToDouble()) {
     truncated->replaceAllUsesWith(truncated->toToDouble()->getOperand(0));
     block->discard(truncated->toToDouble());
   }
 }
 
+bool RangeAnalysis::canTruncate(MDefinition* def, TruncateKind kind) const {
+  if (kind == TruncateKind::NoTruncate) {
+    return false;
+  }
+
+  // Range Analysis is sometimes eager to do optimizations, even if we
+  // are not able to truncate an instruction. In such case, we
+  // speculatively compile the instruction to an int32 instruction
+  // while adding a guard. This is what is implied by
+  // TruncateAfterBailout.
+  //
+  // If a previous compilation was invalidated because a speculative
+  // truncation bailed out, we no longer attempt to make this kind of
+  // eager optimization.
+  if (mir->outerInfo().hadEagerTruncationBailout()) {
+    if (kind == TruncateKind::TruncateAfterBailouts) {
+      return false;
+    }
+    for (uint32_t i = 0; i < def->numOperands(); i++) {
+      if (def->operandTruncateKind(i) <= TruncateKind::TruncateAfterBailouts) {
+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
 // Iterate backward on all instruction and attempt to truncate operations for
 // each instruction which respect the following list of predicates: Has been
 // analyzed by range analysis, the range has no rounding errors, all uses cases
 // are truncating the result.
 //
 // If the truncation of the operation is successful, then the instruction is
 // queue for later updating the graph to restore the type correctness by
 // converting the operands that need to be truncated.
@@ -3136,36 +3164,19 @@ bool RangeAnalysis::truncate() {
             return false;
           }
           break;
         default:;
       }
 
       bool shouldClone = false;
       TruncateKind kind = ComputeTruncateKind(*iter, &shouldClone);
-      if (kind == TruncateKind::NoTruncate) {
-        continue;
-      }
-
-      // Range Analysis is sometimes eager to do optimizations, even if we
-      // are not be able to truncate an instruction. In such case, we
-      // speculatively compile the instruction to an int32 instruction
-      // while adding a guard. This is what is implied by
-      // TruncateAfterBailout.
-      //
-      // If we already experienced an overflow bailout while executing
-      // code within the current JSScript, we no longer attempt to make
-      // this kind of eager optimizations.
-      if (kind <= TruncateKind::TruncateAfterBailouts &&
-          mir->outerInfo().hadEagerTruncationBailout()) {
-        continue;
-      }
 
       // Truncate this instruction if possible.
-      if (!iter->needTruncation(kind)) {
+      if (!canTruncate(*iter, kind) || !iter->needTruncation(kind)) {
         continue;
       }
 
       SpewTruncate(*iter, kind, shouldClone);
 
       // If needed, clone the current instruction for keeping it for the
       // bailout path.  This give us the ability to truncate instructions
       // even after the removal of branches.
@@ -3181,22 +3192,20 @@ bool RangeAnalysis::truncate() {
       if (!worklist.append(*iter)) {
         return false;
       }
     }
     for (MPhiIterator iter(block->phisBegin()), end(block->phisEnd());
          iter != end; ++iter) {
       bool shouldClone = false;
       TruncateKind kind = ComputeTruncateKind(*iter, &shouldClone);
-      if (kind == TruncateKind::NoTruncate) {
-        continue;
-      }
 
       // Truncate this phi if possible.
-      if (shouldClone || !iter->needTruncation(kind)) {
+      if (shouldClone || !canTruncate(*iter, kind) ||
+          !iter->needTruncation(kind)) {
         continue;
       }
 
       SpewTruncate(*iter, kind, shouldClone);
 
       iter->truncate();
 
       // Delay updates of inputs/outputs to avoid creating node which
--- a/js/src/jit/RangeAnalysis.h
+++ b/js/src/jit/RangeAnalysis.h
@@ -33,16 +33,18 @@ class MBasicBlock;
 class MBinaryBitwiseInstruction;
 class MBoundsCheck;
 class MDefinition;
 class MIRGenerator;
 class MIRGraph;
 class MPhi;
 class MTest;
 
+enum class TruncateKind;
+
 // An upper bound computed on the number of backedges a loop will take.
 // This count only includes backedges taken while running Ion code: for OSR
 // loops, this will exclude iterations that executed in the interpreter or in
 // baseline compiled code.
 struct LoopIterationBound : public TempObject {
   // Loop for which this bound applies.
   MBasicBlock* header;
 
@@ -118,16 +120,18 @@ class RangeAnalysis {
   [[nodiscard]] bool analyze();
   [[nodiscard]] bool addRangeAssertions();
   [[nodiscard]] bool removeBetaNodes();
   [[nodiscard]] bool prepareForUCE(bool* shouldRemoveDeadCode);
   [[nodiscard]] bool tryRemovingGuards();
   [[nodiscard]] bool truncate();
   [[nodiscard]] bool removeUnnecessaryBitops();
 
+  bool canTruncate(MDefinition* def, TruncateKind kind) const;
+
   // Any iteration bounds discovered for loops in the graph.
   LoopIterationBoundVector loopIterationBounds;
 
  private:
   [[nodiscard]] bool analyzeLoop(MBasicBlock* header);
   LoopIterationBound* analyzeLoopIterationCount(MBasicBlock* header,
                                                 MTest* test,
                                                 BranchDirection direction);