Change 7694 will affect your change - you need to use the AST id of the unary operation node when doing the CallIC call, and you will look up the IC state and data type using that AST id in the type oracle, instead of using code position to do the lookup.
On Wed, Apr 27, 2011 at 1:34 PM, <[email protected]> wrote: > 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 > -- William Hesse Software Engineer [email protected] Google Denmark ApS Frederiksborggade 20B, 1 sal 1360 København K Denmark CVR nr. 28 86 69 84 If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sender know it went to the wrong person. Thanks. -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
