Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278863002:
[turbofan] Minor code cleanup for builtin inlining (Closed)
Description[turbofan] Minor code cleanup for builtin inlining
BUG=
Committed: https://crrev.com/25f3de99e14a61414887e763425dd9273eb22e93
Cr-Commit-Position: refs/heads/master@{#38896}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments #Messages
Total messages: 16 (10 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jgruber@chromium.org changed reviewers: + jarin@chromium.org
Description was changed from ========== [turbofan] Minor code cleanup for builtin inlining BUG= ========== to ========== [turbofan] Minor code cleanup for builtin inlining BUG= ==========
lgtm. https://codereview.chromium.org/2278863002/diff/1/src/compiler/js-typed-lower... File src/compiler/js-typed-lowering.cc (right): https://codereview.chromium.org/2278863002/diff/1/src/compiler/js-typed-lower... src/compiler/js-typed-lowering.cc:1735: } I am wondering whether it would be better to write this: if (NeedsArgumentAdaptorFrame(shared, arity)) { // Patch {node} to an indirect call via the ArgumentsAdaptorTrampoline. ... } else if (is_builtin && Builtins::HasCppImplementation(builtin_index)) { // Patch {node} to a direct CEntryStub call. ... } else { // Patch {node} to a direct call. ... }
https://codereview.chromium.org/2278863002/diff/1/src/compiler/js-typed-lower... File src/compiler/js-typed-lowering.cc (right): https://codereview.chromium.org/2278863002/diff/1/src/compiler/js-typed-lower... src/compiler/js-typed-lowering.cc:1735: } On 2016/08/25 08:47:09, Jarin wrote: > I am wondering whether it would be better to write this: > > if (NeedsArgumentAdaptorFrame(shared, arity)) { > // Patch {node} to an indirect call via the ArgumentsAdaptorTrampoline. > ... > } else if (is_builtin && Builtins::HasCppImplementation(builtin_index)) { > // Patch {node} to a direct CEntryStub call. > ... > } else { > // Patch {node} to a direct call. > ... > } Good point! Done.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2278863002/#ps20001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Minor code cleanup for builtin inlining BUG= ========== to ========== [turbofan] Minor code cleanup for builtin inlining BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Minor code cleanup for builtin inlining BUG= ========== to ========== [turbofan] Minor code cleanup for builtin inlining BUG= Committed: https://crrev.com/25f3de99e14a61414887e763425dd9273eb22e93 Cr-Commit-Position: refs/heads/master@{#38896} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/25f3de99e14a61414887e763425dd9273eb22e93 Cr-Commit-Position: refs/heads/master@{#38896} |
