Closed Bug 806415 Opened 12 years ago Closed 12 years ago

BaselineCompiler: Clean up FrameInfo

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files)

No description provided.
Renames FrameState to FrameInfo (better name welcome) and moves it to its own file.
Attachment #676186 - Flags: review?(kvijayan)
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+
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.
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.

Attachment

General

Created:
Updated:
Size: