1328826 - Look at baseline stubs to decide to use MConcat

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

Currently we only look at the input types to decide when to use MConcat. If the types don't give enough information we could look to the ics in baseline and if we only see ICBinaryArith_StringConcat we could use MConcat.

Comment 1

[Jan de Mooij [:jandem]]

I think we should also get this with bug 1324561, but if this is a win in the meantime we shouldn't wait for that.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Patch

Totally agree that CacheIR MIR generation will solve this. Though created this super quickly as a result like you said we can probably just take this for now.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 4

[Jan de Mooij [:jandem]]

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

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

Looks good.

::: js/src/jit/MIR.h
@@ -7218,1 @@
>          setMovable();

This is OK because we will bailout if we see an object or symbol?

Comment 5

[Hannes Verschore [:h4writer]]

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

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

::: js/src/jit/MIR.h
@@ -7218,1 @@
>          setMovable();

Correct

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ca0dfe8d72
IonMonkey: Speculate concatenation on baseline caches, r=jandem

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/28ca0dfe8d72

Comment 8

[Hannes Verschore [:h4writer]]

Backout out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc278ae455617ea056ccc9248616811c33d57e0

(In reply to Jan de Mooij [:jandem] from comment #1)
> I think we should also get this with bug 1324561, but if this is a win in
> the meantime we shouldn't wait for that.

We need to test that at least one of the operands are string, which we don't do anymore with this code. Doing that might be a bit more complicated and given above comment I won't spend more time on this.