Closed Bug 1068725 Opened 8 years ago Closed 7 years ago

Odin: Add Mandelbrot and FBirds to the test suite


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: bbouvier, Assigned: bbouvier)




(4 files)

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.
Depends on: 1031203
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)
Attachment #8495207 - Flags: review?(luke) → review+
I just saw comment 1: I'm not sure what you mean, though; how do the iterations or canvas size relate?
(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.
Oh, I see.  Yeah, 1s of computation sounds just fine.
The Mandelbrot demo is actually not working on linux 32-bits. Investigating.
The first bad revision is:
changeset:   204756:179193fbcccd
user:        Benjamin Bouvier <>
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)
(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: Alternatively, view them on Treeherder (experimental):
Flags: needinfo?(sunfish)
Flags: needinfo?(dtc-moz)
- 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)
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 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 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+
Attached patch simd-fbirds-demoSplinter Review
This passes on x86 and x64, locally at least (running a try build to be sure).
Attachment #8497495 - Flags: review?(luke)
Comment on attachment 8497495 [details] [diff] [review]

Attachment #8497495 - Flags: review?(luke) → review+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.