Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1493553007:
PPC: Fix inobject slack tracking for both subclassing and non-subclassing cases. (Closed)
|
Created:
5 years ago by MTBrandyberry Modified:
5 years ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionPPC: Fix inobject slack tracking for both subclassing and non-subclassing cases.
Port 5d38d6819cb872e75b878864d285779959fd8e09
Original commit message:
It didn't support subclassing case at all and in non-subclassing case the runtime
allocation didn't do the slack tracking step.
R=ishell@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
BUG=chromium:563339
LOG=N
Committed: https://crrev.com/23d838f91b24e5dfed4b81cc944840e7cdf6ba23
Cr-Commit-Position: refs/heads/master@{#32591}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Resolve comments #Messages
Total messages: 19 (8 generated)
The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493553007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493553007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nits: https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc File src/ppc/builtins-ppc.cc (right): https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:421: // Check if slack tracking is enabled. This comment should be removed. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:429: __ push(r0); // Save allocation count value. It looks like there are enough registers and r0 is not trashed till line 450. So we can avoid push/pop-ing like in mips ports. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:450: __ pop(r3); // Restore allocation count value before decreasing. Ditto. Remove pop and... https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:451: __ cmpi(r3, Operand(Map::kSlackTrackingCounterEnd)); use r0 instead of r3 here. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:454: // Push the constructor, new target and map to the stack, and Please update this comment.
Thanks for the review! https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc File src/ppc/builtins-ppc.cc (right): https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:421: // Check if slack tracking is enabled. On 2015/12/03 16:47:02, Igor Sheludko wrote: > This comment should be removed. Done. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:429: __ push(r0); // Save allocation count value. On 2015/12/03 16:47:02, Igor Sheludko wrote: > It looks like there are enough registers and r0 is not trashed till line 450. So > we can avoid push/pop-ing like in mips ports. Done. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:450: __ pop(r3); // Restore allocation count value before decreasing. On 2015/12/03 16:47:02, Igor Sheludko wrote: > Ditto. Remove pop and... Done. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:451: __ cmpi(r3, Operand(Map::kSlackTrackingCounterEnd)); On 2015/12/03 16:47:02, Igor Sheludko wrote: > use r0 instead of r3 here. Done. https://codereview.chromium.org/1493553007/diff/1/src/ppc/builtins-ppc.cc#new... src/ppc/builtins-ppc.cc:454: // Push the constructor, new target and map to the stack, and On 2015/12/03 16:47:02, Igor Sheludko wrote: > Please update this comment. Done.
lgtm
The CQ bit was checked by mbrandy@us.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493553007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493553007/20001
The CQ bit was unchecked by mbrandy@us.ibm.com
The CQ bit was checked by mbrandy@us.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493553007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493553007/20001
The CQ bit was unchecked by mbrandy@us.ibm.com
The CQ bit was checked by mbrandy@us.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493553007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493553007/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
PPC: Fix inobject slack tracking for both subclassing and non-subclassing cases.
Port 5d38d6819cb872e75b878864d285779959fd8e09
Original commit message:
It didn't support subclassing case at all and in non-subclassing case the
runtime
allocation didn't do the slack tracking step.
R=ishell@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com,
michael_dawson@ca.ibm.com
BUG=chromium:563339
LOG=N
==========
to
==========
PPC: Fix inobject slack tracking for both subclassing and non-subclassing cases.
Port 5d38d6819cb872e75b878864d285779959fd8e09
Original commit message:
It didn't support subclassing case at all and in non-subclassing case the
runtime
allocation didn't do the slack tracking step.
R=ishell@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com,
michael_dawson@ca.ibm.com
BUG=chromium:563339
LOG=N
Committed: https://crrev.com/23d838f91b24e5dfed4b81cc944840e7cdf6ba23
Cr-Commit-Position: refs/heads/master@{#32591}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/23d838f91b24e5dfed4b81cc944840e7cdf6ba23 Cr-Commit-Position: refs/heads/master@{#32591} |
