(function() { let r g = function(x) { ((-0x80000000 + (x >>> 0)) != 0) ? 0 : x() } })() try { g(NaN) print(g(0x80000000)) } catch (e) {} $ ./js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267 --fuzzing-safe --no-threads --ion-eager testcase.js undefined $ ./js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267 --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis --no-sse3 testcase.js Tested this on m-c rev 89a190592267. My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests It seems to go back beyond Nov 2013 ( https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3 ), so setting needinfo? from Jan and Hannes to see who can move this forward.

The first bad revision is: changeset: 159485:96a2c0c25cab user: Nicolas B. Pierron <nicolas.b.pierron@mozilla.com> date: Mon Dec 09 05:56:19 2013 -0800 summary: Bug 943303 - Annotate and modify conditions leading to dead branches. r=sunfish

@nbp: can you have a look at this? IIUC that change just adds another run of UCE. So the error might be in UCE?

This seems to be that Range Analysis infer a range for MUrsh, which assume that a bailout will occur if the input is not in the Int32 range. Then the condition is inferred based on the Range Analysis, but this baked in constant should still be guarded with the int32-range check of the MUrsh operand. This is one of the case, that I was arguing lately on IRC, where I wished we separated the data-flow from the annotations (guards). As we do want to remove the computation, but still want to execute the guard if it cannot be removed.

Let me detail the problem first, we have the following: 5 Instruction range R5 | Guard filters range R5f 7 Instruction range R7 | Guard filters range R7f 8 Condition 5 7 9 Test 8 <true> <false> The patch I have so far is adding the setup such that we would keep instructions 5 and 7, because both are filtering their inputs. So we removed the condition, but keep the operands which are fallible. The question I wonder, is do we have to keep both instructions? Ideally we would want to keep the condition which might influence the result of the condition. So we should be checking if we could infer the same result if we had R5f & R7, R5 & R7f and R5f & R7f, and only keep the extra range which might change the condition. If none of the extra range are changing the condition, then there is no need to care about these filtered ranges. Another way to deal with this issue would be to have an additional Range to represent the full set of values which might be represented if we had no Int32 representation failures. So if we can infer the same result with the full range, then we have no extra condition to use as guard. Otherwise, we would have to figure which difference contribute to an unknown / different result for the condition.

We eliminate >>>0 bailouts in some circumstances (tryUseUnsignedOperands()), and we could extend this to be more powerful (bug 1116807, bug 1057779, bug 938176, etc.). In many cases, this approach is effective and simple. This doesn't work in the testcase in this bug, but only because there's an add in between the >>>0 and the comparison, and that's probably rare in real-world code. In the long term, to cover all the complex cases of >>>0 bailouts, we probably want to introduce a real uint32 type (bug 750947). Given the above, I don't see it as super important to do clever things to allow us to remove the extra instructions here, unless we have a real-world testcase where it's needed.

This patch change the Range::Range(const MDefinition *) constructor such that we emulate the Uint32 MIRType without changing the output of the instruction. This way, we keep a conservative range for the output of MUrsh, which works well with the Input range of MToDouble, and infer correctly the range of MAdd which cause the previous MCompare to have a non-empty intersection.

Comment on attachment 8553104 [details] [diff] [review] Always consider the range of MUrsh as an unsigned int32. Review of attachment 8553104 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +584,5 @@ > // Simulate the effect of converting the value to its type. > switch (def->type()) { > case MIRType_Int32: > + if (!IsUInt32Def(def)) > + wrapAroundToInt32(); I don't see what makes MUrsh special here. (Let's ignore the bailoutsDisabled() case of MUrsh for the moment, as that's not relevant in the given testcase.) MUrsh can produce values outside the int32 range, and it has bailouts to ensure that its result is in the int32 range. But, this is true of MAdd and many other operators too. It seems to me that the underlying bug is that range analysis is folding a comparison on the assumption that it's being guarded by bailout checks upstream in the def-use graph, and then EliminateDeadCode is deleting the instructions with the bailout checks because they're no longer being used. This seems like a significant problem, and I don't know what the general solution should be. Do we need to treat all fallible() instructions as being Guards?

(In reply to Dan Gohman [:sunfish] from comment #7) > This seems like a significant problem, and I don't know what the general > solution should be. Do we need to treat all fallible() instructions as being > Guards? Yes, but his sounds way too conservative and this might keep a lot of code around, compared to what used to be removed. I guess we can got for this solution first and open a follow-up bug if we notice a big perf issue on the benchmarks :/

Sadly we have no good way to check for fallible instructions. This approach is more complex. It follows the operands on the conditions and flag these instructions as guards if the Range taken after the MIRType differs from the Range of the instruction. This patch should cover issues with overflow issues.

Comment on attachment 8554537 [details] [diff] [review] Flag fallible instructions as guards if used by UCE. Review of attachment 8554537 [details] [diff] [review]: ----------------------------------------------------------------- This makes sense, thanks! ::: js/src/jit/RangeAnalysis.cpp @@ +3267,5 @@ > + > + if (cond->range()) { > + // Filter the range of the instruction based on its MIRType. > + Range typeFilteredRange(cond); > + You can do "if (cond->isGuard()) continue" here, since cond may happen to be a guard already, in which case we don't need to do anything else with it.

Comment on attachment 8554537 [details] [diff] [review] Flag fallible instructions as guards if used by UCE. I tested this patch but I still see a differential testing error: $ ./js-dbg-opt-64-dm-nsprBuild-darwin-1122402-c9-439476037853-95c76c3b0172 --fuzzing-safe --no-threads --ion-eager 1122402.js undefined $ ./js-dbg-opt-64-dm-nsprBuild-darwin-1122402-c9-439476037853-95c76c3b0172 --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis --no-sse3 1122402.js

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11) > I tested this patch but I still see a differential testing error: Thanks Gary for testing, apparently I did not test properly. I will change the test case to expect the TypeError, and double check the iongraph.

Comment on attachment 8554537 [details] [diff] [review] Flag fallible instructions as guards if used by UCE. Review of attachment 8554537 [details] [diff] [review]: ----------------------------------------------------------------- Gary, the test is fixed as expected if I do the following change: ::: js/src/jit/RangeAnalysis.cpp @@ +3282,5 @@ > + } > + > + for (size_t op = 0, e = cond->numOperands(); op < e; op++) { > + MDefinition *operand = cond->getOperand(op); > + if (!DeadIfUnused(operand) || cond->isInWorklist()) typo - fix: s/cond/operand/

Checking iongraph with the previous fix highlight that we do remove the condition as expected and keep the MUrsh as a guard.

(In reply to Nicolas B. Pierron [:nbp] from comment #13) > typo - fix: > s/cond/operand/ > > > + for (size_t op = 0, e = cond->numOperands(); op < e; op++) { Do I fix the typo here? > > + MDefinition *operand = cond->getOperand(op); Or here? > > + if (!DeadIfUnused(operand) || cond->isInWorklist()) Or here? Or all of them? I'm sorry but it is not obvious which lines to change the typo. Please upload a new patch and add the testcases from here and bug 1124448. You might want to check if bug 1126066 is possibly related.