Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488983002:
[Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. (Closed)
Description[Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops.
Ignition does not collect information about lhs and rhs types. It collects
information about the combined type of lhs, rhs and result types. Since ignition
combines the feedback, sometimes we may collect less precise information than
FCG. This impacts performance of some benchmarks like audio-beat-detection. This
cl tries to mitigate this affect by passing only full-codegen feedback when the
combined FCG feedback is same as the feedback collected by ignition.
BUG=
Committed: https://crrev.com/f445f69244743a9aa8c80901ce2a0468378b9035
Cr-Commit-Position: refs/heads/master@{#40888}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Updated comments. #Messages
Total messages: 18 (11 generated)
The CQ bit was checked by mythria@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.
Description was changed from ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It just collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing FCG feedback only, when the combined FCG feedback is same as the feedback collected by ignition. BUG= ========== to ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ==========
Description was changed from ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ========== to ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ==========
mythria@chromium.org changed reviewers: + bmeurer@chromium.org, rmcilroy@chromium.org
Kind of the last cl trying to tune type feedback for crankshaft. This helps audio-oscillator by 18% Also slightly noisy but seem to help box2d and ray-trace. PTAL. Thanks, Mythri https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc File src/type-info.cc (right): https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode280 src/type-info.cc:280: if (combined_type_from_fcg == *left_type) { I am not sure if == is the right way to check equality of AstTypes or if it is better to have (combined_type_from_fcg->Is(*left_type) && (*left_type)->Is(combined_type_from_fcg) is better.
LGTM.
LGTM with some comment nits. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc File src/type-info.cc (right): https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode270 src/type-info.cc:270: // Full-codegen collects lhs and rhs feedback seperately and crankshaft Capitalise Crankshaft, Full-codegen and Ignition (in all comments below) https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode272 src/type-info.cc:272: // feedback has made the feedback less precise, we can use the feedback Can use -> should use https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode273 src/type-info.cc:273: // only from full-codegen. If the union of feedback from full-codegen and Of feedback -> of the feedback https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode274 src/type-info.cc:274: // if it is same as that of ignition, there is no need of combining and if it is same as -> is the same as https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode274 src/type-info.cc:274: // if it is same as that of ignition, there is no need of combining Of combining -> to combine https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode339 src/type-info.cc:339: // feedback from ignition. Same comments as above.
Thanks Ross and Benedikt. I fixed the comments. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc File src/type-info.cc (right): https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode270 src/type-info.cc:270: // Full-codegen collects lhs and rhs feedback seperately and crankshaft On 2016/11/10 08:45:27, rmcilroy wrote: > Capitalise Crankshaft, Full-codegen and Ignition (in all comments below) Done. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode272 src/type-info.cc:272: // feedback has made the feedback less precise, we can use the feedback On 2016/11/10 08:45:27, rmcilroy wrote: > Can use -> should use Done. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode273 src/type-info.cc:273: // only from full-codegen. If the union of feedback from full-codegen and On 2016/11/10 08:45:27, rmcilroy wrote: > Of feedback -> of the feedback Done. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode274 src/type-info.cc:274: // if it is same as that of ignition, there is no need of combining On 2016/11/10 08:45:27, rmcilroy wrote: > Of combining -> to combine Done. https://codereview.chromium.org/2488983002/diff/1/src/type-info.cc#newcode274 src/type-info.cc:274: // if it is same as that of ignition, there is no need of combining On 2016/11/10 08:45:27, rmcilroy wrote: > Of combining -> to combine Done.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2488983002/#ps20001 (title: "Updated 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 ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ========== to ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= ========== to ========== [Crankshaft] Fine tune merging of Ignition and FCG feedback for binary/compare ops. Ignition does not collect information about lhs and rhs types. It collects information about the combined type of lhs, rhs and result types. Since ignition combines the feedback, sometimes we may collect less precise information than FCG. This impacts performance of some benchmarks like audio-beat-detection. This cl tries to mitigate this affect by passing only full-codegen feedback when the combined FCG feedback is same as the feedback collected by ignition. BUG= Committed: https://crrev.com/f445f69244743a9aa8c80901ce2a0468378b9035 Cr-Commit-Position: refs/heads/master@{#40888} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f445f69244743a9aa8c80901ce2a0468378b9035 Cr-Commit-Position: refs/heads/master@{#40888} |
