Closed
Bug 806415
Opened 12 years ago
Closed 12 years ago
BaselineCompiler: Clean up FrameInfo
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(3 files)
16.73 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
8.00 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
15.36 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Renames FrameState to FrameInfo (better name welcome) and moves it to its own file.
Attachment #676186 -
Flags: review?(kvijayan)
Assignee | ||
Comment 2•12 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 3•12 years ago
|
||
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 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/6425ed9c1ff1
https://hg.mozilla.org/projects/ionmonkey/rev/60393dae2947
Leaving this open for other patches.
Assignee | ||
Comment 7•12 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 8•12 years ago
|
||
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+
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
Pushed with comments addressed.
http://hg.mozilla.org/projects/ionmonkey/rev/5cea6640930b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•