function f(x) { print(x >>> 0 !== Math.imul(4294967297, x >>> 0)) } f(0) f(-1) $ ./js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba --fuzzing-safe --no-threads --ion-eager testcase.js false false $ ./js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba --fuzzing-safe --no-threads --baseline-eager testcase.js false true Tested this on m-c rev aa5f8d47a0ba. 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 autoBisect is running.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/e5d631abcd56 user: Tom Schuster date: Sun Oct 05 15:26:40 2014 +0200 summary: Bug 1073576 - Optimize strict compares with equal operands. r=h4writer Tom, is bug 1073576 a likely regressor?

I kind of think the problem is somewhere in Math.imul, because lhs and rhs of the !== should not be equal.

(In reply to Tom Schuster [:evilpie] from comment #2) > I kind of think the problem is somewhere in Math.imul, because lhs and rhs > of the !== should not be equal. Jan/Hannes, how best to move this forward then?

I'll take a look tomorrow morning.

> MAdd maxint, 1 > MTruncateInt32 MAdd > MCompare MAdd, MTruncateInt32 So the issue is that MToTruncateInt32 fold to "MAdd maxint,1". So we get MCompare MAdd, MAdd. But be careful. It is actually: "MCompare MAdd(fallible), MAdd(fallible)". So upon changing this to MCompare true, we shouldn't remove the MAdd(fallible)! Because the check for overflow in the MAdd should get executed. 1) Bug 1122402 comment 8 is talking about treating all fallible options as guards? Which hasn't been fully implemented in that bug... 2) Now I need to recheck if doing this can move stuff above the bailout. In that case it would also lead to problems and solution 1 won't be enough!

Make it possible to distinguish between fallible and infallible functions

Comment on attachment 8562757 [details] [diff] [review] Add fallible functions Review of attachment 8562757 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.cpp @@ +3220,5 @@ > if (lhs() != rhs()) > return false; > > + //lhs()->setGuard(); > + //lhs()->setUseRemovedUnchecked(); Not part of this patch. So remove comments.

Comment on attachment 8562757 [details] [diff] [review] Add fallible functions Review of attachment 8562757 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +1193,5 @@ > return startType_; > } > + > + bool fallible() const MOZ_OVERRIDE { > + return true; I really doubt MStart is fallible. This is approach as we might be too greedy at flagging instructions as fallible. This patch would be fine if it came with a way to warn if we are too greedy at flagging instruction as fallible. Currently the only true way to know if an instruction is fallible or not is to check if the generated code is using a Snapshot. We should also check that the Snapshot is used if it got assigned. And identically for the Lowering, we should check that fallible() is equivalent to assignSnapshot calls.

Comment on attachment 8562760 [details] [diff] [review] Disable some optimization for fallible functions Review of attachment 8562760 [details] [diff] [review]: ----------------------------------------------------------------- Accepting this patch would be a major regression feature wise, and maybe performance wise. Even if the fallible flag was precise and not conservative, this would basically disable any Recover instruction & Truncation, and will keep a lot of code which ought to be removed. ::: js/src/jit/IonAnalysis.cpp @@ +539,5 @@ > bool > js::jit::DeadIfUnused(const MDefinition *def) > { > return !def->isEffectful() && !def->isGuard() && !def->isControlInstruction() && > + !def->fallible() && Even a fallible instruction can be dead, and we might want to remove such fallible instruction. The only valid reason to keep a fallible instruction is if we might have used it for taking a removed branch. ::: js/src/jit/RangeAnalysis.cpp @@ +2799,5 @@ > > + // If the instruction is fallible and has uses removed, one cannot deduce > + // if full truncation is possible by looking at the uses alone. > + if (candidate->fallible() && hasUseRemoved) > + kind = Min(kind, MDefinition::TruncateAfterBailouts); I don't think so. Almost all instructions that are being truncate are fallible, and we should not care as they are truncated. Also, this should already be handled by the conditions which are below. Also if we are truncating, this means that some fallible parts would be removed, and this is better expressed by the 4 Truncate flags. ::: js/src/jit/Sink.cpp @@ +56,5 @@ > MInstruction *ins = *iter++; > > // Only instructions which can be recovered on bailout can be moved > // into the bailout paths. > + if (ins->isGuard() || ins->fallible() || Same thing as DeadIfUnused.

(In reply to Nicolas B. Pierron [:nbp] from comment #12) > Comment on attachment 8562760 [details] [diff] [review] > Disable some optimization for fallible functions > > Review of attachment 8562760 [details] [diff] [review]: > ----------------------------------------------------------------- > > Accepting this patch would be a major regression feature wise, and maybe > performance wise. Even if the fallible flag was precise and not > conservative, this would basically disable any Recover instruction & > Truncation, and will keep a lot of code which ought to be removed. I tried to keep fallible function as precise as possible. Yes there are two or three instructions where it can get improved. Now this might give also an incentive to make instructions less fallible as much as possible! But fallible functions have a hidden path. In most cases we forget that. We think that uses() contain all uses if UseRemoved is not set. For fallible functions that is incorrect. So doing analysis on the uses without taking the "hidden" path into consideration can cause wrong results. For Sink.cpp I disabled every fallible instructions. Since if we cannot be sure that fallible() will not happen and that bailing in that instruction is not observable. I think for almost all instructions it is observable. The bailout path is because we don't support something (type or output or ...). So the code after this instruction assumes that case will not happen. Making this instruction a recover instructions will hide the bailout and possibly hide the assumptions we made... > > ::: js/src/jit/IonAnalysis.cpp > @@ +539,5 @@ > > bool > > js::jit::DeadIfUnused(const MDefinition *def) > > { > > return !def->isEffectful() && !def->isGuard() && !def->isControlInstruction() && > > + !def->fallible() && > > Even a fallible instruction can be dead, and we might want to remove such > fallible instruction. > The only valid reason to keep a fallible instruction is if we might have > used it for taking a removed branch. This is not correct. There is no need for a branch before a fallible function shouldn't get removed: MAdd maxint 1 MTruncate MAdd MCompare MAdd MTruncate MReturn MCompare Will happily get compiled to MAdd maxint 1 MReturn true Removing the MAdd would be very wrong! It guards that the output of MAdd is within the range! Sunfish wrote in bug 1122402 comment 7 that we might want to make all fallible instructions as like they are guards. I agree. > > ::: js/src/jit/RangeAnalysis.cpp > @@ +2799,5 @@ > > > > + // If the instruction is fallible and has uses removed, one cannot deduce > > + // if full truncation is possible by looking at the uses alone. > > + if (candidate->fallible() && hasUseRemoved) > > + kind = Min(kind, MDefinition::TruncateAfterBailouts); > > I don't think so. Almost all instructions that are being truncate are > fallible, and we should not care as they are truncated. > > Also, this should already be handled by the conditions which are below. > Also if we are truncating, this means that some fallible parts would be > removed, and this is better expressed by the 4 Truncate flags. No, the conditions below don't handle this case: MAdd maxint 1 (guarded==true) MTruncate MAdd MCompare MAdd MTruncate MReturn MCompare will get translated into: MAdd maxint 1 MReturn true Nicholas and I debated this online for some time. We both agree it sucks and I think we came to an understanding that this might be needed indeed. > > ::: js/src/jit/Sink.cpp > @@ +56,5 @@ > > MInstruction *ins = *iter++; > > > > // Only instructions which can be recovered on bailout can be moved > > // into the bailout paths. > > + if (ins->isGuard() || ins->fallible() || > > Same thing as DeadIfUnused. Same thing as commented above. We might want to handle fallible instructions more as guards. Like Sunfish proposed.

(In reply to Hannes Verschore [:h4writer] from comment #13) > Nicholas and I debated this online for some time. We both agree it sucks and *Nicolas* > I think we came to an understanding that this might be needed indeed. Not in the current form. We should not change DeadIfUnused, Sink, nor Range Analysis. The idea would be to have the same fix as we added in Bug 1122402, but in MCompare::FoldsTo, which folds MCompare X X to True.

(In reply to Nicolas B. Pierron [:nbp] from comment #14) > (In reply to Hannes Verschore [:h4writer] from comment #13) > > Nicholas and I debated this online for some time. We both agree it sucks and > > *Nicolas* Sorry. Nicolas. I don't know why my mind keeps getting this wrong. > > > I think we came to an understanding that this might be needed indeed. > > Not in the current form. > > We should not change DeadIfUnused, Sink, nor Range Analysis. The idea would > be to have the same fix as we added in Bug 1122402, but in > MCompare::FoldsTo, which folds MCompare X X to True. I was talking about the two specific lines: > > + if (candidate->fallible() && hasUseRemoved) > > + kind = Min(kind, MDefinition::TruncateAfterBailouts); And as far as I understood our talk, you agreed that adding those lines are needed. Even if we decide to take the Bug 1122402 route (which I'm not particularly a fan off, since it looks like adding more band-aid instead of fixing the root problem) for the remaining things. The above two line additions are still required to fix the issue fully.

(In reply to Hannes Verschore [:h4writer] from comment #15) > (In reply to Nicolas B. Pierron [:nbp] from comment #14) > > (In reply to Hannes Verschore [:h4writer] from comment #13) > > > I think we came to an understanding that this might be needed indeed. > > > > Not in the current form. > > > > We should not change DeadIfUnused, Sink, nor Range Analysis. The idea would > > be to have the same fix as we added in Bug 1122402, but in > > MCompare::FoldsTo, which folds MCompare X X to True. > > I was talking about the two specific lines: > > > + if (candidate->fallible() && hasUseRemoved) > > > + kind = Min(kind, MDefinition::TruncateAfterBailouts); > > And as far as I understood our talk, you agreed that adding those lines are > needed. > > Even if we decide to take the Bug 1122402 route (which I'm not particularly > a fan off, since it looks like adding more band-aid instead of fixing the > root problem) for the remaining things. The above two line additions are > still required to fix the issue fully. Then find another way, but having a fallible test at these location*s* is unacceptable. From what I understand, if we follow what has been done in Bug 1122402 we would no longer have to check if the instruction is fallible or not.

For good measurements I tried your proposition. This is a quick and dirty patch implementing it. This is still failing (like I was expecting). But can you have a quick look if I didn't do something super stupid rendering this solution faulty? > $ js --ion-eager --no-threads jit-test/tests/ion/bug1130679.js > jit-test/tests/ion/bug1130679.js:6:0 Error: Assertion failed: got false, expected true

(In reply to Hannes Verschore [:h4writer] from comment #17) > Created attachment 8564081 [details] [diff] [review] > Nicolas' idea > > For good measurements I tried your proposition. This is a quick and dirty > patch implementing it. > This is still failing (like I was expecting). But can you have a quick look > if I didn't do something super stupid rendering this solution faulty? Just to be clear. I will further investigate this path and try to find a solution using that base. Just want to be sure that this is the vanilla implementation you were mentioning.

Comment on attachment 8564081 [details] [diff] [review] Nicolas' idea Review of attachment 8564081 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this is what I meant. ::: js/src/jit/MIR.cpp @@ +3255,5 @@ > + > + for (size_t op = 0, e = cond->numOperands(); op < e; op++) { > + MDefinition *operand = cond->getOperand(op); > + if (!DeadIfUnused(operand) || operand->isInWorklist()) > + continue; Note that DeadIfUnused will always be false here, because of the MReturn use. Thus the operand of the comparison (MAdd) will never be flagged as a guard, and Range Analysis will truncate it. You might want a different stopping condition.

Comment on attachment 8562757 [details] [diff] [review] Add fallible functions Review of attachment 8562757 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r+ assuming that resolving the review comments is straightforward. ::: js/src/jit/MIR.h @@ +1193,5 @@ > return startType_; > } > + > + bool fallible() const MOZ_OVERRIDE { > + return true; I agree with nbp. It seems like we could do something like: for each LInstruction *ins after lowering, MOZ_ASSERT_IF((ins->snapshot() != nullptr) == ins->mirRaw()->fallible()); Does that work? @@ +3032,5 @@ > > AliasSet getAliasSet() const MOZ_OVERRIDE { > return AliasSet::None(); > } > + bool fallible() const MOZ_OVERRIDE { Would you mind adding MOZ_FINAL to at least those classes which are overriding fallible(), so that we don't have to make virtual calls when we know what the class is?

Comment on attachment 8562760 [details] [diff] [review] Disable some optimization for fallible functions Review of attachment 8562760 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonAnalysis.cpp @@ +539,5 @@ > bool > js::jit::DeadIfUnused(const MDefinition *def) > { > return !def->isEffectful() && !def->isGuard() && !def->isControlInstruction() && > + !def->fallible() && Should such an instruction have the isGuard() flag set? ::: js/src/jit/MIR.cpp @@ +3219,5 @@ > { > if (lhs() != rhs()) > return false; > > + lhs()->setUseRemovedUnchecked(); It's not clear what this line is doing. And, it happens even in some cases where no folding is done.

The issue with the previous patch was that 'ranges' weren't available yet to deduce if an instruction could get removed or needed a "Guard" This patch introduces a weaker variant of "Guard": "GuardIfFilters". This works the same as a normal Guard, except that we can remove (and propagate) the "GuardIfFilters" statement when analysis shows that this particular function doesn't filter any types. This fixes the issue observed.

Disable this optimization for older releases.

Comment on attachment 8569132 [details] [diff] [review] Add GuardIfFilters Review of attachment 8569132 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +80,5 @@ > + * depend on that instruction (that made the optimazation possible) anymore. > + * In that case we want guard so that instructions doesn't get removed. > + * Though a little bit less strict than the Guard flag. Since if analysis > + * shows that the function doesn't filter types it is safe to propogate > + * the guard upwards and remove from this instruction. s/optimazation/optimization/ s/want guard so/want to guard so/ s/propogate/propagate/ The current comment took me some time to understand the details, I tried to rephrase it. What do you think of: Flag an instruction to be considered as a Guard if the instructions bails out on some inputs. Otherwise, this flag can be transfered to the operands of this instruction. Some optimizations can replace an instruction, and leave its operands unused. When the type information of the operand got used as a predicate of the transformation, then we have to flag the operands as GuardIfFilters. This flag prevents further optimization of unused instructions, which might remove the run-time checks (bailout conditions) used as a predicate of the previous transformation. ::: js/src/jit/RangeAnalysis.cpp @@ +2798,5 @@ > } > > + // We cannot do full trunction on guarded instructions. > + if (candidate->isGuard() || candidate->isGuardIfFilters()) > + kind = Min(kind, MDefinition::TruncateAfterBailouts); Nice. Also, it seems that every time we check for isGuard, we also check for isGuardIfFilters. Would it make sense to have "isGuard" by default, and then annotate the guard with "isOptimPredicate" ?

Comment on attachment 8569145 [details] [diff] [review] Aurora and up patch Review of attachment 8569145 [details] [diff] [review]: ----------------------------------------------------------------- What is the performance hit of this patch? ::: js/src/jit/MIR.cpp @@ +3215,5 @@ > bool > MCompare::tryFoldEqualOperands(bool *result) > { > + // Temporarily disabled due to bug 1130679. > + return true; why not false?

Comment on attachment 8569145 [details] [diff] [review] Aurora and up patch Review of attachment 8569145 [details] [diff] [review]: ----------------------------------------------------------------- Don't think there will be a huge hit. I don't remember a big speedup with when the original patch landed. ::: js/src/jit/MIR.cpp @@ +3215,5 @@ > bool > MCompare::tryFoldEqualOperands(bool *result) > { > + // Temporarily disabled due to bug 1130679. > + return true; Most definitely it needs to be "false"! Good catch.

Carries over r+ from prev. patch

For aurora patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c6b4339caa4

Comment on attachment 8569872 [details] [diff] [review] Add GuardRangeBailouts Review of attachment 8569872 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, I definitely prefer this approach :)

Comment on attachment 8569873 [details] [diff] [review] Aurora and up patch Approval Request Comment [Feature/regressing bug #]: Bug 1073576, FF35 [User impact if declined]: Possibility to get wrong JS computation results [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c6b4339caa4 [Risks and why]: This patch disables an optimization taking the default (quite elaborate tested) path. So quite low risk. [String/UUID change made/needed]: /

Comment on attachment 8569873 [details] [diff] [review] Aurora and up patch This hasn't quite hit m-c but let's take this simple change to disable an optimization in Beta 2. Beta+ Aurora+