1229396 - Call propagateOOM in loadConstantDouble etc

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

In loadConstantDouble and similar methods, we do:

  dbl->uses.append(CodeOffsetLabel(j.offset()));

We should probably call propagateOOM() where we do this.

Noticed this when reviewing bug 1229057, I think this changed with bug 1181612.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: propagateoom.patch

Indeed. Plus there are a few refactorings we can do here...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.template.getconstant.patch

This unifies getFloat/getDouble/getSimd into a single template function. We have to use two template parameters: one is the Constant type, the other is the Map type (as for Float/Double the hashing policy is the default one, but not for the SimdData one).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.template.constant.patch

Not sure that template aliasing is working (the comment in public/Vector.h suggests it's not, but it mentions MSVC2013 as the starting point from which it could work, and I think that's the minimal version we're using at least, so we should be good). I have got a try push running for this...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.template.constant.merging.patch

Merging constants in the asm.js merging scheme can easily be done with a single function and templates, without depending on patch 3. This eliminates a few redundancies.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Fwiw, the try server build returned all green, even with patch 3. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=119995703da8

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8694277 [details] [diff] [review]
propagateoom.patch

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

Thanks!

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8694316 [details] [diff] [review]
2.template.getconstant.patch

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

Nice refactoring.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +258,5 @@
> +    return &vec[index];
> +}
> +
> +MacroAssemblerX86Shared::Float*
> +MacroAssemblerX86Shared::getFloat(float f) {

Nit: { on next line, also twice below

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8694318 [details] [diff] [review]
4.template.constant.merging.patch

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

Great deduplication.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b964e554709
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a7fcf24c49
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c0681d35b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/904fe1ab68b9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b964e554709
https://hg.mozilla.org/mozilla-central/rev/b7a7fcf24c49
https://hg.mozilla.org/mozilla-central/rev/54c0681d35b0
https://hg.mozilla.org/mozilla-central/rev/904fe1ab68b9