Closed
Bug 1068725
Opened 10 years ago
Closed 10 years ago
Odin: Add Mandelbrot and FBirds to the test suite
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(4 files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
To ensure that bugs like bug 1068331 don't happen again, we should probably 1) make sure that they compile. Any debug build would have caught that; apparently not all opt builds did. 2) make sure that they return consistent results.
Assignee | ||
Comment 1•10 years ago
|
||
I just saw that --tbpl doesn't run the test with --no-asmjs, so maybe I could raise the number of iterations or the canvas size.
Attachment #8495207 -
Flags: review?(luke)
![]() |
||
Updated•10 years ago
|
Attachment #8495207 -
Flags: review?(luke) → review+
![]() |
||
Comment 2•10 years ago
|
||
I just saw comment 1: I'm not sure what you mean, though; how do the iterations or canvas size relate?
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2) > I just saw comment 1: I'm not sure what you mean, though; how do the > iterations or canvas size relate? The overall complexity is O(height * width * iterations). The test runs in ~1 second in all --tbpl modes on my machine, so we could make it longer and add more assertions, if needed. However, it helped me catch a few SIMD interpreter bugs, as a matter of fact it should be enough for just testing that compilation and runtime work.
Assignee | ||
Comment 5•10 years ago
|
||
The Mandelbrot demo is actually not working on linux 32-bits. Investigating.
Assignee | ||
Comment 6•10 years ago
|
||
The first bad revision is: changeset: 204756:179193fbcccd user: Benjamin Bouvier <benj@benj.me> date: Thu Sep 11 08:50:10 2014 +0200 summary: Bug 1051860: Optimize SimdValueX4 codegen for float32x4 with unpcklps; r=sunfish Alignment issue, once more... // start of function [Codegen] instruction Int32x4 [Codegen] pxor %xmm1, %xmm1 [Codegen] instruction MoveGroup [Codegen] movaps 0x50(%esp), %xmm0 [Codegen] instruction SimdSplatX4 [Codegen] shufps 0x0, %xmm0, %xmm0 [Codegen] instruction MoveGroup [Codegen] movss 0x60(%esp), %xmm2 [Codegen] movaps %xmm0, 0x30(%esp) [Codegen] instruction MathF:add [Codegen] addss 0x58(%esp), %xmm2 [Codegen] instruction MoveGroup [Codegen] movss 0x60(%esp), %xmm3 [Codegen] instruction MathF:add [Codegen] addss %xmm2, %xmm3 [Codegen] instruction MoveGroup [Codegen] movss 0x60(%esp), %xmm0 [Codegen] instruction MathF:add [Codegen] addss %xmm3, %xmm0 [Codegen] instruction MoveGroup // issue here. As SimdValueFloat32x4 expects all inputs being float32, // $xmm0, $xmm3, $xmm4, $xmm5 have to be float32. [Codegen] movaps %xmm2, %xmm4 [Codegen] movaps 0x58(%esp), %xmm5 // This move isn't valid! [Codegen] instruction SimdValueFloat32x4 [Codegen] unpcklps %xmm0, %xmm4 [Codegen] unpcklps %xmm3, %xmm5 [Codegen] unpcklps %xmm4, %xmm5 // $xmm5 is reused for the float32x4 output [Codegen] instruction MoveGroup [Codegen] movaps %xmm5, 0x20(%esp) // hence this move is valid Dan, Doug, could it be the same issue as bug 1062067?
Flags: needinfo?(sunfish)
Flags: needinfo?(dtc-moz)
Assignee | ||
Comment 7•10 years ago
|
||
(maybe) Got a fix! As it's related to regalloc and thus pretty sensitive, let's make some try run... remote: You can view the progress of your build at the following URL: remote: https://tbpl.mozilla.org/?tree=Try&rev=a8f48160a81d remote: Alternatively, view them on Treeherder (experimental): remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a8f48160a81d
Flags: needinfo?(sunfish)
Flags: needinfo?(dtc-moz)
Assignee | ||
Comment 8•10 years ago
|
||
- Adds some spew for MoveGroup, indicating the type of a MoveGroup. - Adds some assertions (about alignment of LStackSlot and LArgument) that would have caught this issue at JIT time.
Attachment #8497067 -
Flags: review?(sunfish)
Assignee | ||
Comment 9•10 years ago
|
||
So, here's the issue: 1) a Float32 Add (LMathF) flows into the LSimdValueFloat32x4. 2) LSimdValueFloat32x4 reuses its first input at start. Its output type is Float32x4. 3) When adding a move in the backtracking regalloc reify function when there's a MUST_REUSE_INPUT hint, we create a LMove that uses the output LDefinition::Type as the type of the move. As a matter of fact, we try to move the input (LMathF => float32) with the output move type (the output is LSimdValueFloat32x4 => float32x4). The float32 is 8 bytes off SimdStackAlignment, movaps is used, kaboom. So what this patch tries to do is using the input's LDefinition::Type. I *think* this information is stored in the current VirtualRegister we're looking at, please correct me if I'm wrong. (fwiw, i've built the shell for linux32 and linux64, ran jit-tests and they all pass with this change. Haven't run with --tbpl yet, hence the try run)
Attachment #8497075 -
Flags: review?(sunfish)
Comment 10•10 years ago
|
||
Comment on attachment 8497075 [details] [diff] [review] 2) Use type of the current vreg, rather than the type of the output definition, for movegroups involving MUST_REUSE_INPUT Review of attachment 8497075 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch!
Attachment #8497075 -
Flags: review?(sunfish) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8497067 [details] [diff] [review] 1) More debugging and assertions Review of attachment 8497067 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RegisterAllocator.cpp @@ +409,5 @@ > for (int i = group->numMoves() - 1; i >= 0; i--) { > // Use two printfs, as LAllocation::toString is not reentant. > fprintf(stderr, " [%s", group->getMove(i).from()->toString()); > + fprintf(stderr, " -> %s, ", group->getMove(i).to()->toString()); > + fprintf(stderr, "%s]", LDefinition::TypeToString(group->getMove(i).type())); This is a regalloc dump, which is typically used for looking at virtual register live ranges and allocations, and doesn't have as much information. I think it'd be better to print the type information in LMoveGroup::printOperands, and then you can also use "<%s>" where %s is the TypeChars string, for consistency with how types are printed for other things. Oh, and also add FLOAT32X4 and INT32X4 to TypeChars. Feel free to use strings of length > 1 for them too.
Attachment #8497067 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 12•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/76009dc6ed72 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ed62c1d3f6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc808ef69a4 Now working on the FBirds demo.
Keywords: leave-open
Comment 13•10 years ago
|
||
sorry had to backout this changes for bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=49186882&tree=Mozilla-Inbound
Assignee | ||
Comment 14•10 years ago
|
||
TypeChars is debug only, duh. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5847dc62ad2a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/87a45740b94f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbf9e3e6809
Assignee | ||
Comment 15•10 years ago
|
||
This passes on x86 and x64, locally at least (running a try build to be sure).
Attachment #8497495 -
Flags: review?(luke)
![]() |
||
Comment 16•10 years ago
|
||
Comment on attachment 8497495 [details] [diff] [review] simd-fbirds-demo great!
Attachment #8497495 -
Flags: review?(luke) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Try is super green, so let's land this. https://hg.mozilla.org/integration/mozilla-inbound/rev/007b72fb631e
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/5847dc62ad2a https://hg.mozilla.org/mozilla-central/rev/87a45740b94f https://hg.mozilla.org/mozilla-central/rev/9fbf9e3e6809 https://hg.mozilla.org/mozilla-central/rev/007b72fb631e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•