Does it need any changes to make ARM continue to work?
http://codereview.chromium.org/57052/diff/40/49 File src/register-allocator.h (right): http://codereview.chromium.org/57052/diff/40/49#newcode164 Line 164: StaticType static_type() { return static_type_; } const, since the other getters are. http://codereview.chromium.org/57052/diff/2001/3003 File src/codegen-ia32.cc (right): http://codereview.chromium.org/57052/diff/2001/3003#newcode846 Line 846: bool left_is_string = left.static_type().is_jsstring(); This is a separate change for one of us to make: we really need to be able to inspect the types of frame elements without popping them. In this case (as well as the non-smi case below) we pop then push, which may emit code. http://codereview.chromium.org/57052/diff/2001/3004 File src/codegen-ia32.h (right): http://codereview.chromium.org/57052/diff/2001/3004#newcode436 Line 436: SmiAnalysis* type, Could you line up all the arguments here (which means starting the first one on the next line). http://codereview.chromium.org/57052/diff/2001/3005 File src/jump-target.cc (right): http://codereview.chromium.org/57052/diff/2001/3005#newcode286 Line 286: current.set_static_type(StaticType::unknown()); entry_frame_->elements_[i].set_static_type(StaticType::unknown()) http://codereview.chromium.org/57052/diff/2001/3005#newcode295 Line 295: current.set_static_type(merged_type); And similarly. http://codereview.chromium.org/57052/diff/2001/3010 File src/register-allocator.h (right): http://codereview.chromium.org/57052/diff/2001/3010#newcode58 Line 58: bool is_jsstring() { return static_type_ == STRING_TYPE; } Is there a distinction between string and JS string? I much prefer to read is_string(). http://codereview.chromium.org/57052/diff/2001/3010#newcode63 Line 63: // if (object->IsSmi()) return smi(); // Unused for now. Go ahead and put the commented code in? In any case, this probably deserves a comment reminding that the order of comparisons is important (most specific first). http://codereview.chromium.org/57052/diff/2001/3010#newcode89 Line 89: SMI_TYPE = 0x01, Remove likely smi from the ASCII art? http://codereview.chromium.org/57052/diff/2001/3010#newcode91 Line 91: STRING_TYPE = 0x04 | HEAP_OBJECT_TYPE This is good. http://codereview.chromium.org/57052/diff/2001/3010#newcode155 Line 155: StaticType static_type() { return static_type_; } Go ahead and make this const like the other accessors. http://codereview.chromium.org/57052/diff/2001/3016 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/57052/diff/2001/3016#newcode197 Line 197: // Adjust the stack point downward if necessary. As long as you're fixing my comment, you could change "stack point" to "stack pointer". http://codereview.chromium.org/57052/diff/2001/3018 File src/virtual-frame.cc (right): http://codereview.chromium.org/57052/diff/2001/3018#newcode389 Line 389: // register element, or the new element at frame_index, must be made If we keep the merging of copied registers (see comment below) it should happen here as well. http://codereview.chromium.org/57052/diff/2001/3018#newcode446 Line 446: StaticType merge_type = static_type.merge(element.static_type()); This is a little weird. A register on the frame and a register in an off-frame result should be copies of each other. Neither should be changed, without first breaking the copy and moving one to another place. A failure to have the same static_type might indicate a bug somewhere else. So the question is: is there something that can happen to a result to cause us to lose information about its type without changing its value? http://codereview.chromium.org/57052/diff/2001/3019 File src/virtual-frame.h (right): http://codereview.chromium.org/57052/diff/2001/3019#newcode47 Line 47: class FrameElement { Does this still want BASE_EMBEDDED? http://codereview.chromium.org/57052/diff/2001/3019#newcode56 Line 56: : static_type_(), type_(INVALID), copied_(false), synced_(false) { Won't static_type_() happen without mentioning it? http://codereview.chromium.org/57052/diff/2001/3019#newcode68 Line 68: FrameElement result(MEMORY, no_reg, true); Err, the main point of having an enum for SYNCED/NOT_SYNCED is so that we don't have to read trues and falses at the call sites and remember the sense of the flag... http://codereview.chromium.org/57052/diff/2001/3019#newcode76 Line 76: return FrameElement(REGISTER, reg, is_synced != NOT_SYNCED, static_type); ... so could you move the conversion from enum to bool all the way down into the private constructor? http://codereview.chromium.org/57052/diff/2001/3019#newcode175 Line 175: inline FrameElement(Handle<Object> value, bool is_synced) We don't typically write inline if the def'n is right in the header file. http://codereview.chromium.org/57052 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
