Fold constant numbers in MToInt32

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: andy.zsshen, Unassigned, Mentored)

Tracking

unspecified
mozilla38
x86_64
Windows 8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.99 Safari/537.36
(Reporter)

Updated

4 years ago
Summary: Bug 1102187 - Fold constant numbers in MToInt32 → Fold constant numbers in MToInt32
(Reporter)

Comment 1

4 years ago
This MIR converts a primitive data to the int32 value.

To follow the GVN optimization, we can improve MToInt32:foldsTo() to test if the input operand is a
constant data and replace the original MIR with the MConstant.
Mentor: hv1989

Updated

4 years ago
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
(Reporter)

Comment 2

4 years ago
First post a draft here.

MDefinition *
MToInt32::foldsTo(TempAllocator &alloc)
{
    MDefinition *input = getOperand(0);
    
    // Fold this operation is the input operand is constant.
    if (input->isConstant()) {
        MConstant *constant = NULL;
        switch (input->type()) {
          case MIRType_Null:
            constant = MConstant::New(alloc, Int32Value(0));
            break;
          case MIRType_Int32:
          case MIRType_Float32:
          case MIRType_Double:
            double dval = input->toNumber();
            int32_t ival = static_cast<int32_t>(dval);
            if (dval == ival)
                constant = MConstant::New(alloc, Int32Value(ival));
            break;
        }
        
        if (constant)
            return constant;
    }

    // Original Snippet.
    if (input->type() == MIRType_Int32)
        return input;
    return this;
}

And request some comments to continue the process.
(Reporter)

Comment 3

4 years ago
Update the draft and the test case will be ready soon.

MDefinition *
MToInt32::foldsTo(TempAllocator &alloc)
{
    MDefinition *input = getOperand(0);
    
    // Fold this operation is the input operand is constant.
    if (input->isConstant()) {
        switch (input->type()) {
          case MIRType_Null:
            MOZ_ASSERT(conversion == MacroAssembler::IntConversion_Any);
            return MConstant::New(alloc, Int32Value(0));
          case MIRType_Boolean:
            MOZ_ASSERT(conversion == MacroAssembler::IntConversion_Any);
            MOZ_ASSERT(conversion == MacroAssembler::IntConversion_NumbersOrBoolsOnly);
          case MIRType_Int32:
            Value ival = input->toConstant()->value();
            return MConstant::New(alloc, Int32Value(ival.toInt32()));
          case MIRType_Float32:
          case MIRType_Double:
            double dval = input->toNumber();
            int32_t ival = static_cast<int32_t>(dval);
            // Type cast testing.
            // Only the value within the range of Int32 can be represented as constant.
            if (dval == ival)
                return MConstant::New(alloc, Int32Value(ival));
        }
    }

    // Original Snippet.
    if (input->type() == MIRType_Int32)
        return input;
    return this;
}
(In reply to andy.zsshen from comment #3)
>           case MIRType_Float32:
>           case MIRType_Double:
>             double dval = input->toNumber();
>             int32_t ival = static_cast<int32_t>(dval);
>             // Type cast testing.
>             // Only the value within the range of Int32 can be represented
> as constant.
>             if (dval == ival)
>                 return MConstant::New(alloc, Int32Value(ival));

You want mozilla::NumberEqualsInt32(input->toNumber(), &ival), not an open-coding of it that's buggy in various cases.  (I think this is wrong for any float/double outside of int32_t range, but don't quote me.)  In general try to use existing methods/operations, not reimplement them.
(Reporter)

Comment 5

4 years ago
Created attachment 8552826 [details] [diff] [review]
MToInt32_FoldsTo.patch

More formal implementation.
And the corresponding test cases are also attached.
But I still think for better testing approach.
Attachment #8552826 - Flags: review?(hv1989)
Comment on attachment 8552826 [details] [diff] [review]
MToInt32_FoldsTo.patch

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

Looks good. Please use feedback? if the patch is not complete yet ;)

::: js/src/jit-test/tests/ion/bug1123064.js
@@ +12,5 @@
> +    // Still thinking for better case.
> +    str.charAt(1.1);
> +
> +    // Case3: The type of input operand is constant bool.
> +    // Still thinking.

please complete. Out of curiosity: why did you choose not to use ToInteger?
I think with ToInteger is possible to test null and boolean ;).

::: js/src/jit/MIR.cpp
@@ +2932,5 @@
> +            return MConstant::New(alloc, Int32Value(0));
> +          case MIRType_Boolean: {
> +            MOZ_ASSERT(convert == MacroAssembler::IntConversion_Any ||
> +                       convert == MacroAssembler::IntConversion_NumbersOrBoolsOnly);
> +            Value val = input->toConstant()->value();

can you hoist this val outside the switch. This will remove the {} in most cases.
Attachment #8552826 - Flags: review?(hv1989) → feedback+
(Reporter)

Comment 7

4 years ago
Created attachment 8553066 [details] [diff] [review]
MToInt32_FoldsTo.patch

1. Factor out the statment "Value val = input->toConstant()->value();"
2. Add the test cases for int, double, boolean, and null.
   But for floating point, if the fractional part of the operand is not zero,
   it cannot pass the checking "NumberEqualsInt32". If the fractional part is
   zero, it will be represented as MIRType_Int32.
   Thus I request some advices for better test case.
Attachment #8552826 - Attachment is obsolete: true
Attachment #8553066 - Flags: feedback?(hv1989)
For double:
> function test(a) {
>    return ToInteger(a);
> }
> 
> // Specialize test to take double as input
> test(0.1);
> test(0.1);
> 
> // Run it with integer, which will get converted to double.
> test(0);
> test(0);

For float:
> function test(a) {
>    var b = Math.fround(a);
>    return ToInteger(b);
> }
> 
> // Specialize test to take double as input
> test(0.1);
> test(0.1);
> 
> // Run it with integer, which will get converted to double.
> test(0);
> test(0);
Comment on attachment 8553066 [details] [diff] [review]
MToInt32_FoldsTo.patch

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

::: js/src/jit-test/tests/ion/bug1123064.js
@@ +3,5 @@
> +function toint32() {
> +
> +    // The test case to trigger MToInt32 operation.
> +    var ToInteger = getSelfHostedValue("ToInteger");
> +    

Nit: whitespace
Attachment #8553066 - Flags: feedback?(hv1989) → feedback+
(Reporter)

Comment 10

4 years ago
Created attachment 8553195 [details] [diff] [review]
MToInt32_FoldsTo.patch
Attachment #8553066 - Attachment is obsolete: true
Attachment #8553195 - Flags: feedback?(hv1989)
(Reporter)

Comment 11

4 years ago
These 5 simple cases can hit the corresponding switch handlers of MToInt32::foldsTo().
Comment on attachment 8553195 [details] [diff] [review]
MToInt32_FoldsTo.patch

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

I suspect this patch is ready? So I did a review ;).
Attachment #8553195 - Flags: review+
Comment on attachment 8553195 [details] [diff] [review]
MToInt32_FoldsTo.patch

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

Forgot to remove the feedback request.
Attachment #8553195 - Flags: feedback?(hv1989)
(In reply to andy.zsshen from comment #7)
>    But for floating point, if the fractional part of the operand is not zero,
>    it cannot pass the checking "NumberEqualsInt32". If the fractional part is
>    zero, it will be represented as MIRType_Int32.

This isn't necessarily true.  For asm.js |var f = 0.0;| produces a double, not an int32_t.  If that var's never assigned to again, it could easily be treated as a floating point constant, in the right kinds of code.  I'm not claiming your patch is *definitely* buggy, because I haven't read it.  :-)  I'm just suggesting that those assumptions are probably not valid, and so you *probably* have one somewhere.  Worth doing a double-check to see if you do, and if you do include a test that would have failed under your hypothesis.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> (In reply to andy.zsshen from comment #7)
> >    But for floating point, if the fractional part of the operand is not zero,
> >    it cannot pass the checking "NumberEqualsInt32". If the fractional part is
> >    zero, it will be represented as MIRType_Int32.
> 
> This isn't necessarily true.  For asm.js |var f = 0.0;| produces a double,
> not an int32_t.  If that var's never assigned to again, it could easily be
> treated as a floating point constant, in the right kinds of code.  I'm not
> claiming your patch is *definitely* buggy, because I haven't read it.  :-) 
> I'm just suggesting that those assumptions are probably not valid, and so
> you *probably* have one somewhere.  Worth doing a double-check to see if you
> do, and if you do include a test that would have failed under your
> hypothesis.

I think it wasn't a general statement, but a statement about MToInt32. For MToInt32 it is save to convert an double/float to int32 (if we don't loose precision). If we do loose precision the MToInt32 will bail and IonMonkey will notice the new type and recompile (normally) without a MToInt32.
https://hg.mozilla.org/mozilla-central/rev/07e3fec694b1
https://hg.mozilla.org/mozilla-central/rev/608f0d854e4f
https://hg.mozilla.org/mozilla-central/rev/341afd35e4ee
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1308802
You need to log in before you can comment on or make changes to this bug.