Closed Bug 1229396 Opened 4 years ago Closed 4 years ago

Call propagateOOM in loadConstantDouble etc

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jandem, Assigned: bbouvier)

References

Details

Attachments

(4 files)

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.
Flags: needinfo?(benj)
Indeed. Plus there are a few refactorings we can do here...
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Attachment #8694277 - Flags: review?(jdemooij)
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).
Attachment #8694316 - Flags: review?(jdemooij)
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...
Attachment #8694317 - Flags: review?(jdemooij)
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.
Attachment #8694318 - Flags: review?(jdemooij)
Fwiw, the try server build returned all green, even with patch 3. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=119995703da8
Comment on attachment 8694277 [details] [diff] [review]
propagateoom.patch

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

Thanks!
Attachment #8694277 - Flags: review?(jdemooij) → review+
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
Attachment #8694316 - Flags: review?(jdemooij) → review+
Attachment #8694317 - Flags: review?(jdemooij) → review+
Comment on attachment 8694318 [details] [diff] [review]
4.template.constant.merging.patch

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

Great deduplication.
Attachment #8694318 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.