LNop and LLabel are not needed for backtracking register allocator

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Created attachment 8333879 [details] [diff] [review]
regalloc-trivialize-llabel.patch

LNop and LLabel are two instructions which apparently exist to help out with the way that LSRA does liveness. As far as I can tell, the backtracking allocator doesn't need them.

Attached is a patch which moves the Label out of LLabel and into LBlock, which has the nice effect of simplifying the code in CodeGenerator.cpp, and which makes LLabel a completely trivial instruction, just like LNop.

Then, two patches which disable the creation of LLabel and LNop when the register allocator isn't LSRA. Checking the register allocator flag in a bunch of places is a little ugly, but it does make working in the backtracking allocator a little nicer to not have to worry about whether these extra instructions have any special effect.
(Assignee)

Comment 1

4 years ago
Comment on attachment 8333879 [details] [diff] [review]
regalloc-trivialize-llabel.patch

This patch is just the change to move the Label out of LLabel and into LBlock, which makes some code in CodeGenerator.cpp a little nicer.
Attachment #8333879 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

4 years ago
Created attachment 8336061 [details] [diff] [review]
regalloc-skip-llabel.patch

This patch skips the creation of LLabel when the register allocator is not LSRA. It's a little ugly to be testing for the register allocator everwhere, but it does make debugging the backtracking allocator a little nicer, since there are fewer oddities.
Attachment #8336061 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

4 years ago
Created attachment 8336063 [details] [diff] [review]
regalloc-skip-lnop.patch

Same as the previous, but for LNop.
Attachment #8336063 - Flags: review?(bhackett1024)
Comment on attachment 8333879 [details] [diff] [review]
regalloc-trivialize-llabel.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +2856,5 @@
>          perfSpewer->startBasicBlock(current->mir(), masm);
>  #endif
> +        LInstructionIterator iter = current->begin();
> +
> +        // The first instruction of every block is an LLabel. Skip it.

Maybe point out that label is a nop.  This is purely an optimization, right?
Attachment #8333879 - Flags: review?(bhackett1024) → review+
Comment on attachment 8336061 [details] [diff] [review]
regalloc-skip-llabel.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +2860,5 @@
> +        if (js_IonOptions.registerAllocator == RegisterAllocator_LSRA) {
> +            // The first instruction of every block is an LLabel. Skip it.
> +            JS_ASSERT(iter->isLabel());
> +            ++iter;
> +        }

I think it's better to remove this block entirely, actually.  I think we should be cutting reliance on which of these (process-wide) values is active, as they require everything to line up between Ion/Asm.js, main runtime / worker runtimes, and (most importantly) different levels of optimization for individual scripts.
Attachment #8336061 - Flags: review?(bhackett1024) → review+
Attachment #8336063 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 8336061 [details] [diff] [review]
> regalloc-skip-llabel.patch
> 
> Review of attachment 8336061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +2860,5 @@
> > +        if (js_IonOptions.registerAllocator == RegisterAllocator_LSRA) {
> > +            // The first instruction of every block is an LLabel. Skip it.
> > +            JS_ASSERT(iter->isLabel());
> > +            ++iter;
> > +        }
> 
> I think it's better to remove this block entirely, actually.  I think we
> should be cutting reliance on which of these (process-wide) values is
> active, as they require everything to line up between Ion/Asm.js, main
> runtime / worker runtimes, and (most importantly) different levels of
> optimization for individual scripts.

Makes sense. I removed this block entirely.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1fb7194cb67
https://hg.mozilla.org/integration/mozilla-inbound/rev/45cb9a81f09e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6620d81f76
https://hg.mozilla.org/mozilla-central/rev/f1fb7194cb67
https://hg.mozilla.org/mozilla-central/rev/45cb9a81f09e
https://hg.mozilla.org/mozilla-central/rev/bd6620d81f76
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.