Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551933002:
[turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. (Closed)
|
Created:
4 years ago by Igor Sheludko Modified:
4 years ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com, Michael Hablich Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Add --csa-verify flag that enables machine graph verification of code stubs.
The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now
and once all the verification issues are fixed the flag will be enabled in debug
mode by default.
BUG=
Committed: https://crrev.com/292b3548f6d02b964b4afe3e05f89c0681fa5620
Cr-Commit-Position: refs/heads/master@{#41531}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressing comments #
Messages
Total messages: 29 (22 generated)
Description was changed from ========== [turbofan] Enable machine graph verification for first N stubs. The N is defined by --csa-verify option. BUG= ========== to ========== [turbofan] Enable machine graph verification for first N stubs. ... where N is equal to --csa-verify option. BUG= ==========
The CQ bit was checked by ishell@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...
ishell@chromium.org changed reviewers: + mstarzinger@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a short summary of the offline discussion we had. https://codereview.chromium.org/2551933002/diff/1/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2551933002/diff/1/src/compiler/pipeline.cc#ne... src/compiler/pipeline.cc:1771: --FLAG_csa_verify; suggestion: As discussed offline, this is not safe across multiple isolates (i.e. the flag is per-process, but the pipeline is only synchronized per-isolate). Could we copy out the initial value of FLAG_csa_verify into the {Isolate} when it is initialized and then mutate that value instead (similar to what we do to Heap::allocation_timeout for example)? Or alternatively, have one cctest that sets the flag to a non-zero value, so that we have full control over when this flag is set. We would just not support to pass this flag via Chrome (or other embedders).
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
https://codereview.chromium.org/2551933002/diff/1/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2551933002/diff/1/src/compiler/pipeline.cc#ne... src/compiler/pipeline.cc:1771: --FLAG_csa_verify; On 2016/12/05 18:03:36, Michael Starzinger wrote: > suggestion: As discussed offline, this is not safe across multiple isolates > (i.e. the flag is per-process, but the pipeline is only synchronized > per-isolate). Could we copy out the initial value of FLAG_csa_verify into the > {Isolate} when it is initialized and then mutate that value instead (similar to > what we do to Heap::allocation_timeout for example)? > > Or alternatively, have one cctest that sets the flag to a non-zero value, so > that we have full control over when this flag is set. We would just not support > to pass this flag via Chrome (or other embedders). I added the cctest.
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 ========== [turbofan] Enable machine graph verification for first N stubs. ... where N is equal to --csa-verify option. BUG= ========== to ========== [turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now and once all the verification issues are fixed the flag will be enabled in debug mode by default. BUG= ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ishell@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.
The CQ bit was checked by ishell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2551933002/#ps60001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481040993526860,
"parent_rev": "132142cfa1c0c61a9415312f0b8250e30de1188d", "commit_rev":
"7ef5096c5d7999e5c68b705341b9c3c2fd2481b5"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now and once all the verification issues are fixed the flag will be enabled in debug mode by default. BUG= ========== to ========== [turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now and once all the verification issues are fixed the flag will be enabled in debug mode by default. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now and once all the verification issues are fixed the flag will be enabled in debug mode by default. BUG= ========== to ========== [turbofan] Add --csa-verify flag that enables machine graph verification of code stubs. The flag must be used only by CodeStubAssemblerGraphsCorrectness cctest for now and once all the verification issues are fixed the flag will be enabled in debug mode by default. BUG= Committed: https://crrev.com/292b3548f6d02b964b4afe3e05f89c0681fa5620 Cr-Commit-Position: refs/heads/master@{#41531} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/292b3548f6d02b964b4afe3e05f89c0681fa5620 Cr-Commit-Position: refs/heads/master@{#41531}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/2552893003/ by ishell@chromium.org. The reason for reverting is: Broke nosnap build: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
