1328148 - Use MConcat for more cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Hannes Verschore [:h4writer]]

Looking at pdfjs I saw that we didn't optimize 'str + undefined' or 'str + undefined|str'. We don't support it because our MToString didn't support every case. We now already support more. As a result we should enable MConcat in more cases.

This gives me a 4% on pdfjs.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8823101 [details] [diff] [review]
Patch

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

Good catch.

::: js/src/jit/IonBuilder.cpp
@@ +3221,5 @@
>          trackOptimizationOutcome(TrackedOutcome::OperandNotString);
>          return Ok();
>      }
>  
> +    // The none-string input (if present) should be atleast easily coercible to string.

Pre-existing nit: non-string

@@ +3222,5 @@
>          return Ok();
>      }
>  
> +    // The none-string input (if present) should be atleast easily coercible to string.
> +    if (right->type() != MIRType::String && 

Nit: trailing whitespace. Also can we now remove this check (and for 'left') and just do the mightBeType() calls?

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8823101 [details] [diff] [review]
> Patch
> 
> Review of attachment 8823101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good catch.
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +3221,5 @@
> >          trackOptimizationOutcome(TrackedOutcome::OperandNotString);
> >          return Ok();
> >      }
> >  
> > +    // The none-string input (if present) should be atleast easily coercible to string.
> 
> Pre-existing nit: non-string
> 
> @@ +3222,5 @@
> >          return Ok();
> >      }
> >  
> > +    // The none-string input (if present) should be atleast easily coercible to string.
> > +    if (right->type() != MIRType::String && 
> 
> Nit: trailing whitespace. Also can we now remove this check (and for 'left')
> and just do the mightBeType() calls?

Not really. That would be speculating. This would optimize [string int32] + [string int32] to always use MConcat (which would bail sometimes). We could probably try to specialize based on only seeing ICBinaryArith_StringConcat in the binary ics. Opened bug 1328826 for that

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d8af817497
IonMonkey - Use MConcat for more cases, r=jandem

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/94d8af817497