IonMonkey: rework fixed registers

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Decided not to do this after all. This approach has its own problems and we don't really need this anymore.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 2

6 years ago
We do need this after all, to fix bug 715460 properly. Oh well.
Blocks: 715460
No longer blocks: 709731
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 3

5 years ago
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.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 4

5 years ago
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.
Attachment #588834 - Flags: review?(dvander)
(Assignee)

Comment 5

5 years ago
Created attachment 588835 [details] [diff] [review]
Part 1: Optimize intersect/covers

Oops, forgot to qref.
Attachment #588834 - Attachment is obsolete: true
Attachment #588834 - Flags: review?(dvander)
Attachment #588835 - Flags: review?(dvander)
(Assignee)

Comment 6

5 years ago
Created attachment 588904 [details] [diff] [review]
Part 2: Add fixed intervals

Some other follow-up patches later today.
Attachment #588904 - Flags: review?(dvander)
(Assignee)

Comment 7

5 years ago
Created attachment 588921 [details] [diff] [review]
Part 2: Add fixed intervals
Attachment #588904 - Attachment is obsolete: true
Attachment #588904 - Flags: review?(dvander)
Attachment #588921 - Flags: review?(dvander)
(Assignee)

Comment 8

5 years ago
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.
Attachment #588923 - Flags: review?(dvander)
(Assignee)

Comment 9

5 years ago
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.
Attachment #588835 - Flags: review?(dvander) → review+
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.
Attachment #588921 - Flags: review?(dvander) → review+
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?
Attachment #588923 - Flags: review?(dvander) → review+
(Assignee)

Comment 12

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.