1042823 - Don't blindly make MMinMax optimize for double's

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

In bug 1040196 ToLength is getting selfhosted:

> function ToLength(v) {
>     v = ToInteger(v);
> 
>     if (v <= 0)
>         return 0;
> 
>     // Math.pow(2, 53) - 1 = 9007199254740991
>     return v < 9007199254740991 ? v : 9007199254740991;
> }

ToInteger currently is only implemented as ToInt32(). As a result v will always be in the Int32 range. That means we can remove the "v < large double" check.

This patch does three things:
1) folding of "int32 < large double"
2) folding of "Min(int32, large double)"
3) Be less eager about MMinMax double'ification

(1) and (2) both solve the issue, but happen after type analysis. As a result a MToDouble is added which doesn't get removed.
(3) is even better, since it still keeps the output in integer range.


As a result the code of ToLength is now:

>   movl       0x98(%esp), %eax
>   testl      %eax, %eax
>   jg         nonzero
> zero: 
>   xorl       %eax, %eax
>   movl       %eax, %edx
>   jmp done
> nonzero:
>    movl       %eax, %edx
> done:

If we have range information it can possible also remove the zero case!

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: improve-tolength

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8461004 [details] [diff] [review]
improve-tolength

Review of attachment 8461004 [details] [diff] [review]:
-----------------------------------------------------------------

Nice idea! :)

::: js/src/jit/MCallOptimize.cpp
@@ +1071,5 @@
>      MIRType returnType = getInlineReturnType();
>      if (!IsNumberType(returnType))
>          return InliningStatus_NotInlined;
>  
> +    MDefinitionVector int32_cases(alloc());

Nit: int32Cases

@@ +1083,5 @@
> +            break;
> +          case MIRType_Double:
> +          case MIRType_Float32:
> +            // Don't force a double MMinMax for arguments that would be a NOP
> +            // when doing an integer MMinMax.

Maybe add a sentence to explain NaN is handled properly, in case somebody decides to invert the conditions and forgets about it, something like: "If the argument is NaN, the conditions below will be false and a double comparison is forced."

@@ +1094,5 @@
> +                if (cte <= INT32_MIN && max)
> +                    break;
> +            }
> +
> +            // Force double MMinMax if argument is a "effectfull" double.

Nit: "effectful", but I think it's confusing because we usually use that for something that has side-effects (like def->isEffectful()). Maybe just "if the result could be a double."?

::: js/src/jit/MIR.cpp
@@ +1534,5 @@
> +        if (val.isDouble() && val.toDouble() >= INT32_MAX && !isMax())
> +            return operand;
> +
> +        // max(int32, cte <= INT32_MIN) = int32
> +        if (val.isDouble() && val.toDouble() < INT32_MIN && isMax())

Nit: use <= to match the comment and the code in MCallOptimize.cpp

@@ +2619,5 @@
> +    if (compareType() == Compare_Double) {
> +        // Optimize "MCompare MConstant (MToDouble SomethingInInt32Range).
> +        // In most cases the MToDouble was added, because the constant is
> +        // a double.
> +        // e.g. v < 9007199254740991, where v is an int32 is always true.

Nit: s/is always true/so the result is always true/?

@@ +2626,5 @@
> +
> +        MDefinition *operand = left->isConstant() ? right : left;
> +        MConstant *constant = left->isConstant() ? left->toConstant() : right->toConstant();
> +        JS_ASSERT(constant->value().isDouble());
> +        double cte = constant->value().toDouble();

Nit: what does cte mean? Maybe ctd for "constant double" or just "d"? Same elsewhere.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> > +        // e.g. v < 9007199254740991, where v is an int32 is always true.
> 
> Nit: s/is always true/so the result is always true/?

Sorry I misread the comment, add a comma between "int32" and "is" or maybe:

// e.g. int32 < 9007199254740991 is always true.

Comment 4

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c01dea53dd60

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c01dea53dd60

Comment 6

[Hannes Verschore [:h4writer]]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f814c8559f

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Fix for MCompare

This fixes the MCompare issues, but not the MinMax issues. Same sort of fix can get applied there.

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Fix

This should fix all cases.

Gkw: can you fuzz this on revision adcfd366969e. Esp with Math.min, Math.max, compares, 2147483648 and -2147483648 ?

Comment 9

[Ed Morley [:emorley]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/48f814c8559f

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/48f814c8559f

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8473674 [details] [diff] [review]
Fix

function m(f) {
    try {
        f();
    } catch (e) {}
}
function f(x, y) {
    Math.min(y, ~x)()
};
function g() {
    f(x, 4294967297)
}
m(g)
m(f)
function h() {}
m(h)

Assertion failure: from->type() == to->type() (Def replacement has different type), at jit/ValueNumbering.cpp

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] behind on email from comment #10)
> Assertion failure: from->type() == to->type() (Def replacement has different
> type), at jit/ValueNumbering.cpp

Thanks!

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: mminmax-fix

Gkw: This should fix the given testcase. Can you fuzz further with this patch on revision adcfd366969e. Esp with Math.min, Math.max, compares, 2147483648 and -2147483648 ?

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8474541 [details] [diff] [review]
mminmax-fix

Done - nothing found on an instance of Linux64 debug shell overnight.

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8474541 [details] [diff] [review]
mminmax-fix

Review of attachment 8474541 [details] [diff] [review]:
-----------------------------------------------------------------

This is the extra changes needed to land the previous patch, without bustage.

Comment 16

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #15)
> Comment on attachment 8474541 [details] [diff] [review]
> mminmax-fix
> 
> Review of attachment 8474541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is the extra changes needed to land the previous patch, without bustage.

Thanks!

Comment 17

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT-5 Aug 25-29 from comment #14)
> Comment on attachment 8474541 [details] [diff] [review]
> mminmax-fix
> 
> Done - nothing found on an instance of Linux64 debug shell overnight.

Thanks!

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8474541 [details] [diff] [review]
mminmax-fix

Review of attachment 8474541 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. Forwarding to sunfish though as I'm not very familiar with the RA/truncation/MLimitedTruncate code.

Comment 19

[Dan Gohman [:sunfish]]

Comment on attachment 8474541 [details] [diff] [review]
mminmax-fix

Review of attachment 8474541 [details] [diff] [review]:
-----------------------------------------------------------------

I'm missing some context here. The spec at the revision quoted in the patch defines ToInteger as special cases and then sign(number)  floor(abs(number)). This is not the same as toInt32, so why is our toInteger defined as toInt32? If it's intentional and not temporary, then shouldn't the code in ToLength be simplified by hand rather than relying on the optimizer?

Of course, it's still an optimization we can do in any case.

::: js/src/jit/MIR.cpp
@@ +2736,5 @@
>                default:
>                  MOZ_ASSUME_UNREACHABLE("Unexpected op.");
>              }
> +            if (replaced) {
> +                operand->setGuardUnchecked();

Use of the Guard flag for something that's not a guard in the typical sense is unusual. Doing something unusual among code which is otherwise very ordinary (constant folding is about as ordinary as it gets) magnifies unusualness. I've not yet read through this whole patch and don't yet understand what the bug is or why this fixes it, but if this use of the Guard flag is going to stay, it really wants an explanatory comment.

Comment 20

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman [:sunfish] from comment #19)
> Comment on attachment 8474541 [details] [diff] [review]
> mminmax-fix
> 
> Review of attachment 8474541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm missing some context here. The spec at the revision quoted in the patch
> defines ToInteger as special cases and then sign(number) 
> floor(abs(number)). This is not the same as toInt32, so why is our toInteger
> defined as toInt32? If it's intentional and not temporary, then shouldn't
> the code in ToLength be simplified by hand rather than relying on the
> optimizer?

ToInteger is currently only using a fastpath for Int32 inputs. Else it will take a slower path. This might change in the future, if ToInteger gets used a lot with big integers. We currently don't have something similar in IonMonkey. (Btw probably a good first bug)

ToLength is defined in self-hosting code. We could also make an intrinsic out of it. But it will not gain us a lot. Our engine is good enough to figure this out by itself. So there is no reason to add more code here. And it makes it easier to see it conforms the ecmascript standard.

> 
> Of course, it's still an optimization we can do in any case.
> 
> ::: js/src/jit/MIR.cpp
> @@ +2736,5 @@
> >                default:
> >                  MOZ_ASSUME_UNREACHABLE("Unexpected op.");
> >              }
> > +            if (replaced) {
> > +                operand->setGuardUnchecked();
> 
> Use of the Guard flag for something that's not a guard in the typical sense
> is unusual. Doing something unusual among code which is otherwise very
> ordinary (constant folding is about as ordinary as it gets) magnifies
> unusualness. I've not yet read through this whole patch and don't yet
> understand what the bug is or why this fixes it, but if this use of the
> Guard flag is going to stay, it really wants an explanatory comment.

The guard flag, like mentioned in the documentation, is not specific for "guard" instructions.
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=mir.h&case=true#65
The flag is used to make sure an instruction doesn't get removed even when it doesn't have any uses anymore. That is also what the indention is. We will potentially remove the last use right there. But executing that code can have side-effects (e.g. bail due to recording a type change)

We do this all the time. E.g:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=mir.h&case=true#3694
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=mir.h&case=true#3694

Comment 21

[Dan Gohman [:sunfish]]

(In reply to Hannes Verschore [:h4writer] from comment #20)
> (In reply to Dan Gohman [:sunfish] from comment #19)
> > Comment on attachment 8474541 [details] [diff] [review]
> > mminmax-fix
> > 
> > Review of attachment 8474541 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm missing some context here. The spec at the revision quoted in the patch
> > defines ToInteger as special cases and then sign(number) 
> > floor(abs(number)). This is not the same as toInt32, so why is our toInteger
> > defined as toInt32? If it's intentional and not temporary, then shouldn't
> > the code in ToLength be simplified by hand rather than relying on the
> > optimizer?
> 
> ToInteger is currently only using a fastpath for Int32 inputs. Else it will
> take a slower path. This might change in the future, if ToInteger gets used
> a lot with big integers. We currently don't have something similar in
> IonMonkey. (Btw probably a good first bug)
> 
> ToLength is defined in self-hosting code. We could also make an intrinsic
> out of it. But it will not gain us a lot. Our engine is good enough to
> figure this out by itself. So there is no reason to add more code here. And
> it makes it easier to see it conforms the ecmascript standard.

Ah, thanks!

> > 
> > Of course, it's still an optimization we can do in any case.
> > 
> > ::: js/src/jit/MIR.cpp
> > @@ +2736,5 @@
> > >                default:
> > >                  MOZ_ASSUME_UNREACHABLE("Unexpected op.");
> > >              }
> > > +            if (replaced) {
> > > +                operand->setGuardUnchecked();
> > 
> > Use of the Guard flag for something that's not a guard in the typical sense
> > is unusual. Doing something unusual among code which is otherwise very
> > ordinary (constant folding is about as ordinary as it gets) magnifies
> > unusualness. I've not yet read through this whole patch and don't yet
> > understand what the bug is or why this fixes it, but if this use of the
> > Guard flag is going to stay, it really wants an explanatory comment.
> 
> The guard flag, like mentioned in the documentation, is not specific for
> "guard" instructions.
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=mir.
> h&case=true#65
> The flag is used to make sure an instruction doesn't get removed even when
> it doesn't have any uses anymore. That is also what the indention is. We
> will potentially remove the last use right there. But executing that code
> can have side-effects (e.g. bail due to recording a type change)
> 
> We do this all the time. E.g:
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=mir.
> h&case=true#3694

This code has a comment :-).

Comment 22

[Dan Gohman [:sunfish]]

For some more background on the Guard flag, that line you pointed out above is probably buggy, because modeling side-effects with the Guard flag means that getAliasSet() still returns AliasSet::None(), so AliasAnalysis is probably being fooled. I stand by my earlier claim that using Guard like this is unusual, and deserves to be at least commented.

I'm still working on understanding this patch. The canTruncate_ flag on MToDouble is a little mysterious. Instead of describing a semantic property, it directs a specific optimization pass to avoid performing a specific transformation. Sometimes this is appropriate, but sometimes it's a sign that a bug is being worked around rather than fixed. I haven't yet determined which it is in this case.

I'm also confused by the MLimitedTruncate nodes, since MMinMax doesn't actually implement truncate or operandTruncateKind.

And, I don't know what prompted the backout, so I don't have a sense of what bug this patch is actually fixing.

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Dan Gohman [:sunfish] from comment #22)
> And, I don't know what prompted the backout, so I don't have a sense of what
> bug this patch is actually fixing.

Bug 1053074 prompted the backout.

Comment 24

[Hannes Verschore [:h4writer]]

Short summary of what this bug solves. The idea of this bug is to be able to remove MMinMax or MCompare when
min(MInsInt32, 24567878754434). So a INT32 type and a big double.
We can transform that to "MInsInt32".

Now the isses we saw in "bug 1053074" is because MInsInt32 can get transformed to have different symantic. E.g. truncation or DCE.

(In reply to Dan Gohman [:sunfish] from comment #22)
> For some more background on the Guard flag, that line you pointed out above
> is probably buggy, because modeling side-effects with the Guard flag means
> that getAliasSet() still returns AliasSet::None(), so AliasAnalysis is
> probably being fooled. I stand by my earlier claim that using Guard like
> this is unusual, and deserves to be at least commented.

1) DCE: if the function has no uses it will get removed. Which will remove a whole chain of instructions. Which means we can loose an instructions that set MIRType_Int32 as output, but can bail for a new type.

I'm fine with just adding a comment. But I still don't think the usage of this is unusual.
The AliasSet is only to make sure we don't move instructions above other instructions.
The Guard flag is only used for instructions that shouldn't get removed. It still means we can move them around. (i.e. hoist them). That isn't an issue.

> I'm still working on understanding this patch. The canTruncate_ flag on
> MToDouble is a little mysterious. Instead of describing a semantic property,
> it directs a specific optimization pass to avoid performing a specific
> transformation. Sometimes this is appropriate, but sometimes it's a sign
> that a bug is being worked around rather than fixed. I haven't yet
> determined which it is in this case.

2.1) Truncation: 
The MToDouble will truncate if all uses are int32. Since there are no uses, we will default to truncating the MToDouble. Which can remove the signal of a bailout. That's what this canTruncate_ flag does. Disabling this.

e.g.:
MToDouble (MUrsh X X)
When truncating this will become:
MUrsh (with bailoutsDisabled) X X

As a result we won't bailout anymore.

> I'm also confused by the MLimitedTruncate nodes, since MMinMax doesn't
> actually implement truncate or operandTruncateKind.

2.2) Truncation:

Ok you have a point. We don't have to add MLimitedTruncate always, but it is safer this way. (I'll come back to that).

The actual bug is if "cases.length == 1". In that case we will replace the MMinMax(MInsInt32) with MinsInt32. Which is correct, if we don't truncate. So the MLimitedTruncate is only needed for this.

Now I decided to add it for all, since I can see ourself improving foldsto of MMinMax maybe, that doesn't take into account this issue. If we have MLimitedTruncate for all inputs, there is no problem of removing the MMinMax if we ever do so.

(Thinking about it, we might fix 2.1. possibly by also using MLimitedTruncate, instead of 'abusing' MToDouble)

> 
> And, I don't know what prompted the backout, so I don't have a sense of what
> bug this patch is actually fixing.

Indeed bug 1053074

Comment 25

[Dan Gohman [:sunfish]]

Attachment: virtual-fallible.patch

I apologize for taking so long with this. Attached is an alternative approach that I'd like to propose. It applies on top of the improve-tolength patch, and it fixes the bug that the mminmax-fix patch fixes, without using guard flags or special truncation disabling.

It makes the fallible() flag, which many MIR classes already implement, virtual, so that it can be tested on an MDefinition. This allows e.g. MCompare::foldsTo to make decisions based on the type() of an operand which may lead to that operand becoming dead. If the operand is fallible(), its type() cannot be trusted for such purposes.

Comment 26

[Dan Gohman [:sunfish]]

Comment on attachment 8474541 [details] [diff] [review]
mminmax-fix

Review of attachment 8474541 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling the review for now, as I've proposed an alternative patch for discussion.

Comment 27

[Hannes Verschore [:h4writer]]

Hmm, I haven't looked too closely yet. Didn't had the time yet. But had already some possible issues:
1) This won't fix the doublification of MinMax. The result of MinMax will be typed Double. So we will do double math instead of int math if we use this. That's the reason for the IonBuilder changes!
2) fallible(), might not be enough. what if we have MToInt32 -> MXXX -> MMinMax. Now assume MXXX is infallible. In that case we will optimize to MXXXX, which is possible to truncate and will go upwards ...
So being bogus.

Comment 28

[Dan Gohman [:sunfish]]

My patch is meant to go with the existing improve-tolength patch here. It's just an alternative to the mminmax-fix patch. The improve-tolength patch has the main IonBuilder changes.

In "MToInt32 -> MXXX -> MMinMax", if MXXX is infallible, we know that it's never going to bail out to return some non-int32 value in baseline or whatever. This is all that MMinMax needs to know to be able to assume that the actual value always be within [INT32_MIN,INT32_MAX], allowing it to be optimized.

Comment 29

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman [:sunfish] from comment #28)
> My patch is meant to go with the existing improve-tolength patch here. It's
> just an alternative to the mminmax-fix patch. The improve-tolength patch has
> the main IonBuilder changes.

Ah sorry. I misunderstood.

> In "MToInt32 -> MXXX -> MMinMax", if MXXX is infallible, we know that it's
> never going to bail out to return some non-int32 value in baseline or
> whatever. This is all that MMinMax needs to know to be able to assume that
> the actual value always be within [INT32_MIN,INT32_MAX], allowing it to be
> optimized.

Yes, I agree it should get optimized. But it still doesn't fix the truncation problem:

MToInt32 -> MXXX -> MMinMax -> MBitAnd

so this gets transformed to:

MToInt32 -> MXXX -> MBitAnd

Which truncation will go to:

MTruncateToInt32 -> MXXX -> MBitAnd

As a result MToInt32 won't bail anymore for doubles, which will yield incorrect results. Since our optimization to remove the double constant was based on the input being integer (not truncated integer). That's the reason we need to disable truncation above the removed MMinMax...

Another solution would be to check all inputs aren't fallible (recursively), but that would mean this optimization probably wouldn't happen in most cases. The "disallow truncation" seems simpler to me.

Comment 30

[Dan Gohman [:sunfish]]

(In reply to Hannes Verschore [:h4writer] from comment #29)
> Yes, I agree it should get optimized. But it still doesn't fix the
> truncation problem:
> 
> MToInt32 -> MXXX -> MMinMax -> MBitAnd
> 
> so this gets transformed to:
> 
> MToInt32 -> MXXX -> MBitAnd
> 
> Which truncation will go to:
> 
> MTruncateToInt32 -> MXXX -> MBitAnd

I don't see how this last step can happen. The MXXX is infallible, so truncating it doesn't change its result value. It's already returning an int32 in all cases, so truncating it doesn't change it in any way. Consequently, truncation doesn't flow through it. In other words, either the MXXX truncates its operands on its own, or it never truncates its operands.

Comment 31

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman [:sunfish] from comment #30)
> (In reply to Hannes Verschore [:h4writer] from comment #29)
> > Yes, I agree it should get optimized. But it still doesn't fix the
> > truncation problem:
> > 
> > MToInt32 -> MXXX -> MMinMax -> MBitAnd
> > 
> > so this gets transformed to:
> > 
> > MToInt32 -> MXXX -> MBitAnd
> > 
> > Which truncation will go to:
> > 
> > MTruncateToInt32 -> MXXX -> MBitAnd
> 
> I don't see how this last step can happen. The MXXX is infallible, so
> truncating it doesn't change its result value. It's already returning an
> int32 in all cases, so truncating it doesn't change it in any way.
> Consequently, truncation doesn't flow through it. In other words, either the
> MXXX truncates its operands on its own, or it never truncates its operands.

No truncating doesn't change the exact value of MXXX, but since MXXX truncates, operands above MXXX can also truncate. As a result e.g. MConstant 6000000000, might get truncated to MConstant 1705032704

So changing MConstant 6000000000 -> MXXX -> MMinMax -> MBitAnd to
MConstant 6000000000 -> MXXX -> MBitAnd

Without putting a MLimitTruncate in between, might become:
MConstant 1705032704 -> MXXX -> MBitAnd

So MXXX wasn't fallible and still receives another value. Since it doesn't limit truncation. So that's the reason for MMinMax to get replaces by MLimitTruncate.

Another issue I have with using "fallible()" is that it will most likely be false during GVN, since most cases where we eliminate "fallible()" is during RA. As a result testing "fallible()" is inferior to using MLimitTruncate. Since it works for fallible() MIRs.

Comment 32

[Dan Gohman [:sunfish]]

(In reply to Hannes Verschore [:h4writer] from comment #31)
> (In reply to Dan Gohman [:sunfish] from comment #30)
> > (In reply to Hannes Verschore [:h4writer] from comment #29)
> > > Yes, I agree it should get optimized. But it still doesn't fix the
> > > truncation problem:
> > > 
> > > MToInt32 -> MXXX -> MMinMax -> MBitAnd
> > > 
> > > so this gets transformed to:
> > > 
> > > MToInt32 -> MXXX -> MBitAnd
> > > 
> > > Which truncation will go to:
> > > 
> > > MTruncateToInt32 -> MXXX -> MBitAnd
> > 
> > I don't see how this last step can happen. The MXXX is infallible, so
> > truncating it doesn't change its result value. It's already returning an
> > int32 in all cases, so truncating it doesn't change it in any way.
> > Consequently, truncation doesn't flow through it. In other words, either the
> > MXXX truncates its operands on its own, or it never truncates its operands.
> 
> No truncating doesn't change the exact value of MXXX, but since MXXX
> truncates, operands above MXXX can also truncate. As a result e.g. MConstant
> 6000000000, might get truncated to MConstant 1705032704
>
> So changing MConstant 6000000000 -> MXXX -> MMinMax -> MBitAnd to
> MConstant 6000000000 -> MXXX -> MBitAnd
> 
> Without putting a MLimitTruncate in between, might become:
> MConstant 1705032704 -> MXXX -> MBitAnd

I don't think this is possible. If the MXXX cares about the untruncated value of the MConstant, then range analysis can't truncate it, even if the MXXX itself is being truncated. If the MXXX doesn't care, the MConstant may be truncated, and it doesn't matter.

> 
> So MXXX wasn't fallible and still receives another value. Since it doesn't
> limit truncation. So that's the reason for MMinMax to get replaces by
> MLimitTruncate.
> 
> Another issue I have with using "fallible()" is that it will most likely be
> false during GVN, since most cases where we eliminate "fallible()" is during
> RA. As a result testing "fallible()" is inferior to using MLimitTruncate.
> Since it works for fallible() MIRs.

Ah, this is true, and in fact it looks like it might affect some of the main use cases here.

MLimitTruncate nodes and extra Guard flags inhibits other optimizations too. If we later discover that the MMinMax or the MCompare is unused, Guard flags on their operands would needlessly prevent their operands from being removed.

I'm continuing to think about this.

Comment 33

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman [:sunfish] from comment #32)
> (In reply to Hannes Verschore [:h4writer] from comment #31)
> > (In reply to Dan Gohman [:sunfish] from comment #30)
> > > (In reply to Hannes Verschore [:h4writer] from comment #29)
> > > > Yes, I agree it should get optimized. But it still doesn't fix the
> > > > truncation problem:
> > > > 
> > > > MToInt32 -> MXXX -> MMinMax -> MBitAnd
> > > > 
> > > > so this gets transformed to:
> > > > 
> > > > MToInt32 -> MXXX -> MBitAnd
> > > > 
> > > > Which truncation will go to:
> > > > 
> > > > MTruncateToInt32 -> MXXX -> MBitAnd
> > > 
> > > I don't see how this last step can happen. The MXXX is infallible, so
> > > truncating it doesn't change its result value. It's already returning an
> > > int32 in all cases, so truncating it doesn't change it in any way.
> > > Consequently, truncation doesn't flow through it. In other words, either the
> > > MXXX truncates its operands on its own, or it never truncates its operands.
> > 
> > No truncating doesn't change the exact value of MXXX, but since MXXX
> > truncates, operands above MXXX can also truncate. As a result e.g. MConstant
> > 6000000000, might get truncated to MConstant 1705032704
> >
> > So changing MConstant 6000000000 -> MXXX -> MMinMax -> MBitAnd to
> > MConstant 6000000000 -> MXXX -> MBitAnd
> > 
> > Without putting a MLimitTruncate in between, might become:
> > MConstant 1705032704 -> MXXX -> MBitAnd
> 
> I don't think this is possible. If the MXXX cares about the untruncated
> value of the MConstant, then range analysis can't truncate it, even if the
> MXXX itself is being truncated. If the MXXX doesn't care, the MConstant may
> be truncated, and it doesn't matter.

The MXXX doesn't care if it receives the truncated/untruncated value. The fault isn't in MXXX. The problem is MMinMax cares about it. So if we want to remove MMinMax, we need to carefully make sure that the code without MMinMax behaves the same as if MMinMax was there. Which means disabling truncation.

Note: thinking again about this patch, I think I can remove the setGuard/disableTruncation flags and only use "MLimitTruncation" for all cases. Which might be better, I think ...

Comment 34

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Hannes Verschore [:h4writer] from comment #33)
> The MXXX doesn't care if it receives the truncated/untruncated value. The
> fault isn't in MXXX. The problem is MMinMax cares about it. So if we want to
> remove MMinMax, we need to carefully make sure that the code without MMinMax
> behaves the same as if MMinMax was there. Which means disabling truncation.

Or saving the original result in the resume points, such as Baseline can redo the computation on its own and obtain the same result.

This sounds like a uses case for the UseRemoved flag, which exactly state that we should inhibit Range Analysis if the result is observable and that it cannot be recovered on bailout.

Comment 35

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8499811 [details] [diff] [review]
virtual-fallible.patch

Review of attachment 8499811 [details] [diff] [review]:
-----------------------------------------------------------------

I do not think this is the right approach knowing that a lot of instructions are fallible.
I wonder if we can make use of the UseRemoved flag / MLimitedTruncate to solve this issue.

Note that the limited truncate is still needed to keep the bailout if we were suppose to catch a Double as input of MMinMax, while the UseRemoved flag is used to recover the original result of removed operands of MMinMax.

  MMinMax(x1, x2) should fold to MLimitedTruncate(x?) and flag all others operands as UseRemoved.

Comment 36

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #35)
>   MMinMax(x1, x2) should fold to MLimitedTruncate(x?) and flag all others
> operands as UseRemoved.

+1 on that. That is also my perception.

Comment 37

[Hannes Verschore [:h4writer]]

Attachment: Use limitedTruncate

This does what nbp suggested in comment 35. Only using MFilteredTruncate. This rmeoves the need of adjusting MToDouble.

Comment 38

[Hannes Verschore [:h4writer]]

Attachment: Use limitedTruncate

forgot to qref

Comment 39

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8504643 [details] [diff] [review]
Use limitedTruncate

Review of attachment 8504643 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MCallOptimize.cpp
@@ +1194,5 @@
>  
>      if (cases.length() == 1) {
> +        MLimitedTruncate *limit = MLimitedTruncate::New(alloc(), cases[0], MDefinition::NoTruncate);
> +        current->add(limit);
> +        current->push(limit);

I am not sure how the truncation will change the value which is returned if there is only one argument.  Thus, I am not sure to understand why we need a MLimitedTruncate here.

::: js/src/jit/MIR.cpp
@@ +1829,5 @@
>          // max(int32, cte <= INT32_MIN) = int32
> +        if (val.isDouble() && val.toDouble() < INT32_MIN && isMax()) {
> +            MLimitedTruncate *limit =
> +                MLimitedTruncate::New(alloc, operand, MDefinition::NoTruncate);
> +            block()->insertBefore(this, limit);

add

constant->setUseRemovedUnchecked();

@@ +3050,5 @@
>              }
> +            if (replaced) {
> +                MLimitedTruncate *limit = MLimitedTruncate::New(alloc, operand, MDefinition::NoTruncate);
> +                limit->setGuardUnchecked();
> +                block()->insertBefore(this, limit);

operand->setUseRemovedUnchecked();
constant->setUseRemovedUnchecked();

instead of these 3 lines?

Comment 40

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #39)
> Comment on attachment 8504643 [details] [diff] [review]
> Use limitedTruncate
> 
> Review of attachment 8504643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1194,5 @@
> >  
> >      if (cases.length() == 1) {
> > +        MLimitedTruncate *limit = MLimitedTruncate::New(alloc(), cases[0], MDefinition::NoTruncate);
> > +        current->add(limit);
> > +        current->push(limit);
> 
> I am not sure how the truncation will change the value which is returned if
> there is only one argument.  Thus, I am not sure to understand why we need a
> MLimitedTruncate here.

This is for min(int, +double); The cases will only contain 1 case. The "int" case. The "+double" was pruned.

Comment 41

[Dan Gohman [:sunfish]]

Comment on attachment 8504643 [details] [diff] [review]
Use limitedTruncate

Review of attachment 8504643 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.cpp
@@ +3050,5 @@
>              }
> +            if (replaced) {
> +                MLimitedTruncate *limit = MLimitedTruncate::New(alloc, operand, MDefinition::NoTruncate);
> +                limit->setGuardUnchecked();
> +                block()->insertBefore(this, limit);

I don't think nbp's idea here is sufficient, because UseRemoved doesn't prevent operand from being dead-code-eliminated, which would also eliminate the bailout checks. Does that make sense?

In either case, please add a comment to this code.

Comment 42

[Hannes Verschore [:h4writer]]

Comment on attachment 8499811 [details] [diff] [review]
virtual-fallible.patch

Review of attachment 8499811 [details] [diff] [review]:
-----------------------------------------------------------------

I think this patch isn't needed anymore.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0520d8ed4ed

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0520d8ed4ed

Comment 44

[Grant Galitz]

did this rebust https://bugzilla.mozilla.org/show_bug.cgi?id=1054169?