712278 - IonMonkey: rework fixed registers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Our current requirement system seems to be too complicated. One of the problems is that we can't spill all intervals at calls, because the spill intervals overlap with the intervals for the output regs. Last week I also had to workaround some issues with must-reuse-input for bug 709731.

We should do what the paper describes and give every physical register its own interval, and use register hints to avoid unnecessary moves.

Comment 1

[Jan de Mooij [:jandem]]

Decided not to do this after all. This approach has its own problems and we don't really need this anymore.

Comment 2

[Jan de Mooij [:jandem]]

We do need this after all, to fix bug 715460 properly. Oh well.

Comment 3

[Jan de Mooij [:jandem]]

We decided to fix this after all. One use case is the following:

1: v1 = ...;
2: divi (temp: edx)
3: use(v1)

Currently we'll happily use edx for the result of the first operation, even though it's evicted immediately for the divi temp. My WIP patch passes jit-tests, fixes spectral-norm with --ion and fixes a few other jit-tests with --ion-eager (-n).

The only problem is that the patch regresses md5 a bit, since there are more ranges now. There are fewer *intervals*, due to removing bogus intervals. This is a good thing because ranges require less memory than intervals. Regressing md5 is unacceptable though, after sstangl's optimization work.

Fortunately, this doesn't seem too hard to fix - since intervals are processed in ascending order, we can do some caching to prevent looping over ranges that are definitely out of range.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 1: Optimize intersect/covers

This patch is necessary to not regress SS md5 (regalloc time for the largest function is 1.7-1.8 ms with or without the patches I'm going to attach). md5 is a kind of worst-case benchmark, since the fixed intervals have many small ranges, caused by the large number of calls.

Intervals are handled in ascending order, so this patch caches the last processed range and tries to start from there the next time covers/intersect is called.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1: Optimize intersect/covers

Oops, forgot to qref.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add fixed intervals

Some other follow-up patches later today.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add fixed intervals

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3: Follow-up fixes

Various fixes necessary to pass jit-tests , most notably:

- Rename the "before" movegroup to "input" movegroup, and add a new "before" movegroup.

- If an instruction uses defineFixed, and the next instruction uses this value with a fixed policy, we'd have to deal with a lot of complexity like fixedReg -> fixedReg moves etc. Fixed output regs are not very common so the patch just adds an LNop instruction after such instructions, so that regalloc does not have to worry about this case.

Comment 9

[Jan de Mooij [:jandem]]

dvander, we should talk about safepoints, too. I think they will need some changes, but not entirely sure which cases we need to handle etc.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 588921 [details] [diff] [review]
Part 2: Add fixed intervals

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

Nice work.

::: js/src/ion/LinearScan.cpp
@@ +549,5 @@
> +                    CodePosition from;
> +                    if (def->policy() == LDefinition::PRESET && def->output()->isRegister()) {
> +                        AnyRegister reg = def->output()->toRegister();
> +                        addFixedRange(reg, inputOf(*ins), outputOf(*ins).next());
> +                        from = outputOf(*ins).next();

Why does the output get a range covering the next instruction?

::: js/src/ion/Lowering.cpp
@@ +168,2 @@
>                                           argslot,
>                                           tempFixed(ArgumentsRectifierReg),

Should have a JS_STATIC_ASSERT that ArgumentsRectifierReg != CallTempReg*?

::: js/src/ion/x64/Assembler-x64.h
@@ +85,5 @@
>  static const Register ScratchReg = r11;
>  static const FloatRegister ScratchFloatReg = { JSC::X86Registers::xmm15 };
>  
>  static const Register ArgumentsRectifierReg = { JSC::X86Registers::r8 };
> +static const Register CallTempReg1 = { rbp };

If you can get away with not using rbp/ebp, I'd avoid it, just because it trips up gdb when stepping through.

::: js/src/ion/x86/Assembler-x86.h
@@ +69,5 @@
>  static const FloatRegister ScratchFloatReg = { JSC::X86Registers::xmm7 };
>  
>  static const Register ArgumentsRectifierReg = { JSC::X86Registers::esi };
> +static const Register CallTempReg1 = { JSC::X86Registers::ebp };
> +static const Register CallTempReg2 = { JSC::X86Registers::eax };

Don't forget ARM.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 588923 [details] [diff] [review]
Part 3: Follow-up fixes

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

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +101,5 @@
>      LDefinition def(type, LDefinition::PRESET);
>      def.setOutput(output);
>  
> +    // Add an LNop to avoid regalloc problems if the next op uses this value
> +    // with a fixed or at-start policy.

What exactly is the problem with this situation?

Comment 12

[Jan de Mooij [:jandem]]

http://hg.mozilla.org/projects/ionmonkey/rev/d326b4261278
http://hg.mozilla.org/projects/ionmonkey/rev/dc878a1566f0
http://hg.mozilla.org/projects/ionmonkey/rev/da22e1665505

Just noticed that these patches fix the v8-v5/check-splay.js crash.

(In reply to David Anderson [:dvander] from comment #10)
> Comment on attachment 588921 [details] [diff] [review]
> 
> Why does the output get a range covering the next instruction?

We discussed this on IRC and I added a comment.

> 
> ::: js/src/ion/Lowering.cpp
> @@ +168,2 @@
> >                                           argslot,
> >                                           tempFixed(ArgumentsRectifierReg),
> 
> Should have a JS_STATIC_ASSERT that ArgumentsRectifierReg != CallTempReg*?

Added these asserts. GCC 4.2 did not like the JS_STATIC_ASSERT with registers (no problems with Clang), so I used a normal JS_ASSERT.

> 
> If you can get away with not using rbp/ebp, I'd avoid it, just because it
> trips up gdb when stepping through.

Changed them to rdi/edi.