BaselineCompiler: Clean up FrameInfo




7 years ago
7 years ago


(Reporter: jandem, Assigned: jandem)


Firefox Tracking Flags

(Not tracked)



(3 attachments)



7 years ago
No description provided.

Comment 1

7 years ago
Renames FrameState to FrameInfo (better name welcome) and moves it to its own file.
Attachment #676186 - Flags: review?(kvijayan)

Comment 2

7 years ago
Add an overview comment, some asserts and fix some small issues.

Note that we have to use AlignedStorage2<ValueOperand> instead of ValueOperand since ValueOperand has no default constructor. We do something similar already in TypedOrValueRegister etc.
Attachment #676188 - Flags: review?(kvijayan)
Comment on attachment 676186 [details] [diff] [review]
Part 1: Rename FrameState to FrameInfo, move to its own file

Review of attachment 676186 [details] [diff] [review]:

Looks good.
Attachment #676186 - Flags: review?(kvijayan) → review+
Comment on attachment 676188 [details] [diff] [review]
Part 2: Add comment, asserts

Review of attachment 676188 [details] [diff] [review]:

::: js/src/ion/BaselineCompiler.cpp
@@ +361,5 @@
>      while (true) {
>          SPEW_OPCODE();
>          JSOp op = JSOp(*pc);
> +        frame.assertValidState(pc);

This should be wrapped in #ifdef DEBUG

::: js/src/ion/BaselineFrameInfo.h
@@ +65,5 @@
>          struct {
>              Value v;
>          } constant;
>          struct {
> +            AlignedStorage2<ValueOperand> reg;

DO we really want to switch away from two/one registers here (depending on arch) to a ValueOperand?

ValueOperand implicitly carries with it the notion that the referred value might have an implicit type tag, which won't ever be the case here.

It seemed cleaner the way it was before: when we constructed the ValueOperand explicitly during the call to reg(), and keeping its internal representation as one/two registers.
Attachment #676188 - Flags: review?(kvijayan)
Comment on attachment 676188 [details] [diff] [review]
Part 2: Add comment, asserts

Review of attachment 676188 [details] [diff] [review]:

My mistake confusing ValueOperand with TypedOrValueRegister.
Attachment #676188 - Flags: review+

Comment 7

7 years ago
This patch replaces some bogus JS_NOT_REACHED calls and fixes all known FrameInfo bugs/problems. This will conflict with your x64 patch, I can rebase on top of that.
Attachment #676652 - Flags: review?(kvijayan)
Comment on attachment 676652 [details] [diff] [review]
Part 3: Clean up, fixes

Review of attachment 676652 [details] [diff] [review]:

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +199,5 @@
>          moveValue(src, dest.valueReg());
>      }
> +    void moveValue(const ValueOperand &src, const ValueOperand &dest) {
> +        movq(src.valueReg(), dest.valueReg());
> +    }

This can do the check for |src.valueReg() == dest.valueReg()| and skip the move if true.  That'll allow elimination of the check in BaselineFrameInfo.cpp (popValue).

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +102,5 @@
> +        JS_ASSERT(src.typeReg() != dest.payloadReg());
> +        JS_ASSERT(src.payloadReg() != dest.typeReg());
> +        movl(src.typeReg(), dest.typeReg());
> +        movl(src.payloadReg(), dest.payloadReg());
> +    }

This can check for |src.typeReg() == dest.typeReg()| and omit the first move, and check for |src.payloadReg() == dest.payloadReg()| and skip the second move.

I just noticed you were doing this in BaselineCompiler before you moved this routine to the assembler.
Attachment #676652 - Flags: review?(kvijayan) → review+
Go ahead and push this, I've put it in the front of my patch queue and am building my other refactors on top of it.

Comment 10

7 years ago
Pushed with comments addressed.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.