Last Comment Bug 712278 - IonMonkey: rework fixed registers
: IonMonkey: rework fixed registers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem] (PTO until July 31)
:
Mentors:
Depends on:
Blocks: 715460
  Show dependency treegraph
 
Reported: 2011-12-20 06:01 PST by Jan de Mooij [:jandem] (PTO until July 31)
Modified: 2012-01-19 02:19 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Optimize intersect/covers (8.13 KB, patch)
2012-01-16 02:41 PST, Jan de Mooij [:jandem] (PTO until July 31)
no flags Details | Diff | Splinter Review
Part 1: Optimize intersect/covers (8.08 KB, patch)
2012-01-16 02:47 PST, Jan de Mooij [:jandem] (PTO until July 31)
dvander: review+
Details | Diff | Splinter Review
Part 2: Add fixed intervals (40.03 KB, patch)
2012-01-16 08:52 PST, Jan de Mooij [:jandem] (PTO until July 31)
no flags Details | Diff | Splinter Review
Part 2: Add fixed intervals (41.81 KB, patch)
2012-01-16 09:52 PST, Jan de Mooij [:jandem] (PTO until July 31)
dvander: review+
Details | Diff | Splinter Review
Part 3: Follow-up fixes (19.23 KB, patch)
2012-01-16 10:09 PST, Jan de Mooij [:jandem] (PTO until July 31)
dvander: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] (PTO until July 31) 2011-12-20 06:01:36 PST
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] (PTO until July 31) 2012-01-06 02:39:26 PST
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] (PTO until July 31) 2012-01-06 04:10:19 PST
We do need this after all, to fix bug 715460 properly. Oh well.
Comment 3 Jan de Mooij [:jandem] (PTO until July 31) 2012-01-13 11:56:11 PST
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] (PTO until July 31) 2012-01-16 02:41:52 PST
Created attachment 588834 [details] [diff] [review]
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] (PTO until July 31) 2012-01-16 02:47:00 PST
Created attachment 588835 [details] [diff] [review]
Part 1: Optimize intersect/covers

Oops, forgot to qref.
Comment 6 Jan de Mooij [:jandem] (PTO until July 31) 2012-01-16 08:52:19 PST
Created attachment 588904 [details] [diff] [review]
Part 2: Add fixed intervals

Some other follow-up patches later today.
Comment 7 Jan de Mooij [:jandem] (PTO until July 31) 2012-01-16 09:52:36 PST
Created attachment 588921 [details] [diff] [review]
Part 2: Add fixed intervals
Comment 8 Jan de Mooij [:jandem] (PTO until July 31) 2012-01-16 10:09:41 PST
Created attachment 588923 [details] [diff] [review]
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] (PTO until July 31) 2012-01-16 10:17:44 PST
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] 2012-01-17 17:34:46 PST
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] 2012-01-17 18:25:23 PST
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] (PTO until July 31) 2012-01-19 02:19:19 PST
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.

Note You need to log in before you can comment on or make changes to this bug.