Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Description • 10 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive) 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.

	Luke Wagner [:luke]
	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?

	Benjamin Bouvier [:bbouvier] (inactive) 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.

	Luke Wagner [:luke]
	Comment 4 • 10 years ago

Oh, I see.  Yeah, 1s of computation sounds just fine.

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 5 • 10 years ago

The Mandelbrot demo is actually not working on linux 32-bits. Investigating.

	Benjamin Bouvier [:bbouvier] (inactive) 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?

	Benjamin Bouvier [:bbouvier] (inactive) 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

	Benjamin Bouvier [:bbouvier] (inactive) 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.

	Benjamin Bouvier [:bbouvier] (inactive) 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)

	Dan Gohman [: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!

	Dan Gohman [:sunfish]
	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.

	Benjamin Bouvier [:bbouvier] (inactive) 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.

	Carsten Book [:Tomcat]
	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

	Benjamin Bouvier [:bbouvier] (inactive) 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

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 15 • 10 years ago

This passes on x86 and x64, locally at least (running a try build to be sure).

	Luke Wagner [:luke]
	Comment 16 • 10 years ago

Comment on attachment 8497495 [details] [diff] [review]
simd-fbirds-demo

great!

	Benjamin Bouvier [:bbouvier] (inactive) Assignee
	Comment 17 • 10 years ago

Try is super green, so let's land this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/007b72fb631e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 10 years ago

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