Looks pretty good now. LGTM with some small comments.
I'm also fine if you want to address some as a separate change, if this cl is
getting too large. http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3744 src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings in Comment would be nice here... On 2011/04/27 00:45:54, Sven wrote:
On 2011/04/26 13:32:52, fschneider wrote: > It's a little complicated to do, since format strings require
allocating a
> buffer in JS heap since comments are referenced by the reloc-info...
- so I'm
> not sure if you should make it a TODO now.
I thought about using something like vsnprintf in the Comment
constructor, this
shouldn't be too hard, I think or am I missing something here?
As Kevin said, we do something similar in Crankshaft (lithium-*) where we print the Lithium-IR as comments. http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3752 src/ia32/full-codegen-ia32.cc:3752: VisitForAccumulatorValue(expr->expression()); On 2011/04/27 00:45:54, Sven wrote:
On 2011/04/26 13:32:52, fschneider wrote: > You need to call here: > > SetSourcePosition(expr->position()) > > This will record the statement position which we use to associate
the type
> feedback with. We have the same call to SetSourcePosition for binary operations.
Done, for x64 and ARM, too.
Why is Expression::position() not pure? Using UNREACHABLE just hides
semantic
errors when a subclass doesn't properly implement this. Another
possibility
might be moving the pos_ field to Expression itself, making position()
a
"normal" method there.
I agree that it would be a good idea to make it abstract. We don't want to store positions for AST nodes where it is not needed however. Feel free to do it as a separate change - otherwise this changelist would grow too large. http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3753 src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub); On 2011/04/27 00:45:54, Sven wrote:
On 2011/04/26 13:32:52, fschneider wrote: > You should use here: > > EmitCallIC(stub.GetCode(), NULL) > > where NULL signals that there is no inlined smi code. > Since we probably want to add patchable inline smi code later (like
we already
> do for binary ops)
I have to admit that I still don't understand this kind of patching in
detail,
but I've changed this like you said. :-) Of course, the x64 and ARM
parts have
been changed accordingly, too. Note that IMHO this triple change is a
strong
hint that there is a bit too much copy-n-paste in the code...
Have a look at uses of JumpPatchSite for BinaryOperation. It's an abstraction for a conditional branch that gets patched on the first execution of the IC stub. It's probably good to add inlined smi code for SUB and BIT_NOT as a separate change. http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (left): http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc#oldcode3733 src/ia32/full-codegen-ia32.cc:3733: GenericUnaryOpStub stub(Token::SUB, overwrite, NO_UNARY_FLAGS); Please remove the code for the GenericUnaryOpStub (in code-stubs.h, etc.), since it is not used anymore after your change. http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc#newcode455 src/x64/code-stubs-x64.cc:455: void TypeRecordingUnaryOpStub::GenerateSmiStubSub(MacroAssembler* masm) { I'm not convinced that all the short helpers like this one, that are used exactly in one place, buy us much. If you plan to refactor these helpers anyway, it's ok to do this as a separate change later. http://codereview.chromium.org/6879081/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
