939820 - LNop and LLabel are not needed for backtracking register allocator

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 1

[Dan Gohman [:sunfish]]

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.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: regalloc-skip-lnop.patch

Same as the previous, but for LNop.

Comment 4

[Brian Hackett [Laid off!]]

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?

Comment 5

[Brian Hackett [Laid off!]]

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.

Comment 6

[Dan Gohman [:sunfish]]

(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

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f1fb7194cb67
https://hg.mozilla.org/mozilla-central/rev/45cb9a81f09e
https://hg.mozilla.org/mozilla-central/rev/bd6620d81f76