Thanks for the review. Landed.
https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc File src/arm/deoptimizer-arm.cc (right): https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc#newcode373 src/arm/deoptimizer-arm.cc:373: // The top address for the bottommost output frame can be computed from On 2012/01/24 00:08:54, Kevin Millikin wrote:
The first half of this comment can be dropped. Suggest:
"The top address of the frame is computed from the previous frame's
top and this
frame's size."
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc#newcode391 src/arm/deoptimizer-arm.cc:391: // Compute caller's PC On 2012/01/24 00:08:54, Kevin Millikin wrote:
Read caller's PC from the previous frame.
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc#newcode401 src/arm/deoptimizer-arm.cc:401: // Compute caller's FP On 2012/01/24 00:08:54, Kevin Millikin wrote:
Read caller's FP from the previous frame, and set this frame's FP.
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc#newcode413 src/arm/deoptimizer-arm.cc:413: // For the bottommost output frame the context can be gotten from the input On 2012/01/24 00:08:54, Kevin Millikin wrote:
Comment is just wrong:
"A marker value is used in place of the context."
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/arm/deoptimizer-arm.cc#newcode436 src/arm/deoptimizer-arm.cc:436: // Number of incomming arguments. On 2012/01/24 00:08:54, Kevin Millikin wrote:
incomming => incoming
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/deoptimizer.h File src/deoptimizer.h (right): https://chromiumcodereview.appspot.com/9265004/diff/4001/src/deoptimizer.h#newcode439 src/deoptimizer.h:439: StackFrame::Type GetType() const { return type_; } On 2012/01/24 00:08:54, Kevin Millikin wrote:
Type and Kind are too generic (kind was before). Suggest GetFrameType/SetFrameType and GetCodeKind/SetCodeKind.
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/ia32/deoptimizer-ia32.cc File src/ia32/deoptimizer-ia32.cc (right): https://chromiumcodereview.appspot.com/9265004/diff/4001/src/ia32/deoptimizer-ia32.cc#newcode442 src/ia32/deoptimizer-ia32.cc:442: void Deoptimizer::DoComputeArgumentsAdaptorFrame(TranslationIterator* iterator, On 2012/01/24 00:08:54, Kevin Millikin wrote:
My suggestions about the comments in the ARM version mostly apply here
as well. Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/ia32/deoptimizer-ia32.cc#newcode451 src/ia32/deoptimizer-ia32.cc:451: unsigned fixed_frame_size = 5 * kPointerSize; On 2012/01/24 00:08:54, Kevin Millikin wrote:
You could compute this as
StandardFrameConstants::kFixedFrameSize + kPointerSize
(with a comment that the extra pointer is the arg count (?)). Or
better, add a
named constant to ArgumentsAdaptorFrameConstants.
Done. https://chromiumcodereview.appspot.com/9265004/diff/4001/src/ia32/frames-ia32.h File src/ia32/frames-ia32.h (right): https://chromiumcodereview.appspot.com/9265004/diff/4001/src/ia32/frames-ia32.h#newcode100 src/ia32/frames-ia32.h:100: static const int kFixedFrameSize = 4; On 2012/01/24 00:08:54, Kevin Millikin wrote:
Currently unused, nice. Maybe a comment is needed here that we're
counting the
marker, context, caller fp, and caller pc as the fixed fields.
Done. https://chromiumcodereview.appspot.com/9265004/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
