Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406843002:
[Interpreter] Collect feedback about Oddballs in Subtract Stub. (Closed)
Description[Interpreter] Collect feedback about Oddballs in Subtract Stub.
Also include feedback about Oddballs when collecting the type feedback.
For now, Number and NumberOrOddball are collected separately
because crankshaft does not handle NumberOrOddballs consistently.
This should change once we fix crankshaft.
BUG=v8:4280, v8:5400
LOG=N
Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458
Committed: https://crrev.com/65716011492fce55475f6e57fc3e8d588afede88
Cr-Original-Commit-Position: refs/heads/master@{#40124}
Cr-Commit-Position: refs/heads/master@{#40170}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed comments from Leszek. #Patch Set 3 : Trying with a landmine. #
Messages
Total messages: 34 (20 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...
mythria@chromium.org changed reviewers: + bmeurer@chromium.org, leszeks@chromium.org
Hi Benedikt and Leszek, I added support to collect feedback about Oddballs to Subtract stub. This stub for now collects Numbers and Numbers+Oddballs as two different types. Both these types are passed to turbofan as Numbers+Oddballs. So Turbofan side things should be good. As discussed today morning, I am investigating if we can modify crankshaft to deal with Oddballs. Once we modify that we no longer need to collect them as two different types of feedbacks. I am sending this out for review because I dint want to delay turbofan investigations to be delayed because of crankshaft. If you think this is Ok, I can implement the same thing for other binary operations. PTAL. Thanks, Mythri
LGTM from my side.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode840 src/code-stubs.cc:840: if_lhsisnotnumber(assembler), check_rhsisoddball(assembler), move the new label definitions to the above label definition list (the one with do_fsub etc) so that these once are only the ones for the branch. https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode944 src/code-stubs.cc:944: Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler); nit: move these declarations to be next to the branch (to match the surrounding code style) https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode958 src/code-stubs.cc:958: assembler->Goto(&call_subtract_stub); if lhs is an oddball, can we not extract its float value and jump to the fsub (with different type feedback of course) rather than calling the stub? https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode972 src/code-stubs.cc:972: assembler->Goto(&call_subtract_stub); as above https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode987 src/code-stubs.cc:987: assembler->Goto(&call_subtract_stub); as above https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode1000 src/code-stubs.cc:1000: assembler->Int32Constant(BinaryOperationFeedback::kAny)); this needs to be removed https://codereview.chromium.org/2406843002/diff/1/src/globals.h File src/globals.h (right): https://codereview.chromium.org/2406843002/diff/1/src/globals.h#newcode1226 src/globals.h:1226: kAny = 0xF Should kAny change to 0x1F, so that it has a value beyond kNumberOrOddball | kString (i.e. so that it has a bit set for non-number, non-strings)? https://codereview.chromium.org/2406843002/diff/1/src/type-info.cc File src/type-info.cc (right): https://codereview.chromium.org/2406843002/diff/1/src/type-info.cc#newcode196 src/type-info.cc:196: AstType* CompareOpHintToType(CompareOperationHint hint) { Should this have a similar change to below?
Thanks Leszek and Benedikt. I addressed comments from Leszek. PTAL. Thanks, Mythri https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode840 src/code-stubs.cc:840: if_lhsisnotnumber(assembler), check_rhsisoddball(assembler), On 2016/10/10 11:57:22, Leszek Swirski wrote: > move the new label definitions to the above label definition list (the one with > do_fsub etc) so that these once are only the ones for the branch. Thanks, Done. https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode944 src/code-stubs.cc:944: Label if_rhsissmi(assembler), if_rhsisnotsmi(assembler); On 2016/10/10 11:57:22, Leszek Swirski wrote: > nit: move these declarations to be next to the branch (to match the surrounding > code style) Done. https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode958 src/code-stubs.cc:958: assembler->Goto(&call_subtract_stub); On 2016/10/10 11:57:22, Leszek Swirski wrote: > if lhs is an oddball, can we not extract its float value and jump to the fsub > (with different type feedback of course) rather than calling the stub? As discussed offline, we cannot directly go to fsub. We could still load the to_number value to save few checks in the subtractStub, but that would involve additional checks in the check_rhsisoddball case. Assuming Oddballs are not very frequent for binary operations, this may not give us a performance win as well. Similar to what we noticed when string add is inlined to Add stub. https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode972 src/code-stubs.cc:972: assembler->Goto(&call_subtract_stub); On 2016/10/10 11:57:22, Leszek Swirski wrote: > as above reply above. https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode987 src/code-stubs.cc:987: assembler->Goto(&call_subtract_stub); On 2016/10/10 11:57:22, Leszek Swirski wrote: > as above reply above https://codereview.chromium.org/2406843002/diff/1/src/code-stubs.cc#newcode1000 src/code-stubs.cc:1000: assembler->Int32Constant(BinaryOperationFeedback::kAny)); On 2016/10/10 11:57:22, Leszek Swirski wrote: > this needs to be removed oops. I missed it. Thanks, we might have wasted time investigating performance regressions. Done. https://codereview.chromium.org/2406843002/diff/1/src/globals.h File src/globals.h (right): https://codereview.chromium.org/2406843002/diff/1/src/globals.h#newcode1226 src/globals.h:1226: kAny = 0xF On 2016/10/10 11:57:22, Leszek Swirski wrote: > Should kAny change to 0x1F, so that it has a value beyond kNumberOrOddball | > kString (i.e. so that it has a bit set for non-number, non-strings)? Good point. Done. https://codereview.chromium.org/2406843002/diff/1/src/type-info.cc File src/type-info.cc (right): https://codereview.chromium.org/2406843002/diff/1/src/type-info.cc#newcode196 src/type-info.cc:196: AstType* CompareOpHintToType(CompareOperationHint hint) { On 2016/10/10 11:57:22, Leszek Swirski wrote: > Should this have a similar change to below? Yes, I think so. I was planning to do with along with changes for compare bytecode handler.
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...
Great, LGTM from me as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.chromium.org/2406843002/#ps20001 (title: "Addressed comments from Leszek.")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. BUG=v8:4280, v8:5400 LOG=N ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2407923002/ by mythria@chromium.org. The reason for reverting is: breaks win32-debug bot..
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
Is the windows error authentic? We had recently other weird windows errors that pointed to a build dependency problem. This looks a bit different though, as the failures were consistent and stopped consistently on the revert. Only this looks fishy: https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds... I'll reopen this CL and try running the win debug trybots on it.
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ==========
Hmja, looks like we have proof for a build deps problem. The second tryjob ran on the same VM that was just red without the landmine: vm261-m4.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ==========
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ==========
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, leszeks@chromium.org Link to the patchset: https://codereview.chromium.org/2406843002/#ps40001 (title: "Trying with a landmine.")
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 ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Cr-Commit-Position: refs/heads/master@{#40124} ========== to ========== [Interpreter] Collect feedback about Oddballs in Subtract Stub. Also include feedback about Oddballs when collecting the type feedback. For now, Number and NumberOrOddball are collected separately because crankshaft does not handle NumberOrOddballs consistently. This should change once we fix crankshaft. BUG=v8:4280, v8:5400 LOG=N Committed: https://crrev.com/d5d283b27d406486e2f8d7b44c6d2b3db4f98458 Committed: https://crrev.com/65716011492fce55475f6e57fc3e8d588afede88 Cr-Original-Commit-Position: refs/heads/master@{#40124} Cr-Commit-Position: refs/heads/master@{#40170} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/65716011492fce55475f6e57fc3e8d588afede88 Cr-Commit-Position: refs/heads/master@{#40170} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
