Closed Bug 1068725 Opened 8 years ago Closed 7 years ago

Odin: Add Mandelbrot and FBirds to the test suite

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(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 <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)
(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)
- 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]
simd-fbirds-demo

great!
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.