Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714193002:
Remove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler) (Closed)
|
Created:
3 years, 10 months ago by vabr (Chromium) Modified:
3 years, 9 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionRemove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler)
Currently, ArrayIncludes handles the hypothetical case of an array with
a fast ElementsKind and non-SMI length. This should not happen (and is checked
against in JSArray::JSArrayVerify of objects_debug.cc).
Therefore this CL replaces that handling with a CSA_ASSERT that the length is
indeed SMI.
The CL also simplifies loading of the (SMI) length on 64 bit architectures by
using LoadAndUntagObjectField instead of LoadObjectField+SmiToWord.
BUG=v8:5985
Review-Url: https://codereview.chromium.org/2714193002
Cr-Commit-Position: refs/heads/master@{#43433}
Committed: https://chromium.googlesource.com/v8/v8/+/72a4922509a9e32da8ec2706e3b6b6cb46ef340a
Patch Set 1 #Patch Set 2 : Assert on tagged + fix compilation #Patch Set 3 : No 'ToWord32' #
Total comments: 3
Patch Set 4 : Remove len_var #Patch Set 5 : Ensure RawMachineAssembler::AddNode does not complain about null current_block_ #
Total comments: 3
Patch Set 6 : No braces #Patch Set 7 : Add const #Messages
Total messages: 40 (28 generated)
The CQ bit was checked by vabr@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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by vabr@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: Try jobs failed on following builders: v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
The CQ bit was checked by vabr@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...
Description was changed from ========== [WIP] Remove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler) Currently, CodeStubAssembler handles the hypothetical case of an array with a fast ElementsKind and non-SMI length. This should not happen (and is checked against in JSArray::JSArrayVerify of objects_debug.cc). Therefore this CL replaces that handling with a CSA_ASSERT that the length is indeed SMI. The CL also simplifies loading of the (SMI) length on 64 bit architectures by using LoadAndUntagToWord32ObjectField instead of LoadObjectField+SmiToWord. BUG=v8:5985 ========== to ========== Remove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler) Currently, ArrayIncludes handles the hypothetical case of an array with a fast ElementsKind and non-SMI length. This should not happen (and is checked against in JSArray::JSArrayVerify of objects_debug.cc). Therefore this CL replaces that handling with a CSA_ASSERT that the length is indeed SMI. The CL also simplifies loading of the (SMI) length on 64 bit architectures by using LoadAndUntagObjectField instead of LoadObjectField+SmiToWord. BUG=v8:5985 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vabr@chromium.org changed reviewers: + bmeurer@chromium.org, caitp@igalia.com
Hi Benedikt and caitp@, This is the other CL we talked about. It is ready for review if you would like to review it. Cheers, Vaclav
LGTM I'm not sure if we want to keep using IntPtr operations in the rest of the algorithm, since they're misleading. https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1710: len_var.Bind(len); Do we still need `len_var`? It never changes at any point in the algorithm, I think
Thanks! I have 2 questions below. :) Cheers, Vaclav https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1710: len_var.Bind(len); On 2017/02/25 18:03:06, caitp wrote: > Do we still need `len_var`? It never changes at any point in the algorithm, I > think So that I learn more: (1) Changing would mean that there is a len_var.Bind() call somewhere after this line? (2) If I remove len_var, then I should keep the Node* pointer (len) and replace all len_var.value() calls with it, correct?
https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/40001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1710: len_var.Bind(len); On 2017/02/25 18:35:48, vabr (Chromium) wrote: > On 2017/02/25 18:03:06, caitp wrote: > > Do we still need `len_var`? It never changes at any point in the algorithm, I > > think > > So that I learn more: > (1) Changing would mean that there is a len_var.Bind() call somewhere after this > line? There's another Bind() call before this line, but IIRC the intent was just to make the compiler happy having a bound value for len in the call_runtime block (which doesn't use it). So, as far as I can tell (based on a quick look), this is the only Bind that does anything useful (e.g. changes the value meaningfully). > (2) If I remove len_var, then I should keep the Node* pointer (len) and replace > all len_var.value() calls with it, correct? Yeah, I think that would be better.
The CQ bit was checked by vabr@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...
Thanks for the answer, caitp@! Looking at the code, I agree that there seems to be no place where |len_var.value()| is called while the value is anything else than the length. So I removed |len_var| as suggested. The bots are green, but I'm not sure about whether I did the assignment to |len| in the right place (see my comment below). Any comments from you are welcome. Benedikt, I will also wait for your review and of course OWNERS approval here. Thanks to both of you. Vaclav P.S. While I enjoyed your quick help and advice on this topic, please let me emphasise that this is not urgent, especially not over the weekend. :) https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1701: Node* len = nullptr; I tried to initialize |len| with LoadAndUntagObjectField directly, after line 1713 (see patch set 4), but RawMachineAssembler::AddNode complained that its |current_block_| is null. I'm not sure how that notion of a block corresponds to the structure of the code here, but assigning the result of LoadAndUntagObjectField inside the code block on lines 1703-1713 helped.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/25 21:56:45, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. LGTM with not. Thanks!
On 2017/02/26 12:18:45, Benedikt Meurer wrote: > On 2017/02/25 21:56:45, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > LGTM with not. Thanks! Thanks, Benedikt! If you meant "with nit" then please also publish your comments. :) Cheers, Vaclav
Meh, sorry... done :-) https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1701: Node* len = nullptr; On 2017/02/25 21:25:55, vabr (Chromium) wrote: > I tried to initialize |len| with LoadAndUntagObjectField directly, after line > 1713 (see patch set 4), but RawMachineAssembler::AddNode complained that its > |current_block_| is null. I'm not sure how that notion of a block corresponds to > the structure of the code here, but assigning the result of > LoadAndUntagObjectField inside the code block on lines 1703-1713 helped. Just remove the { and } in 1704, and declare the Node* Len with the assignment below.
The CQ bit was checked by vabr@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 checked by vabr@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 vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com, bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2714193002/#ps120001 (title: "Add const")
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": 120001, "attempt_start_ts": 1488138870599850,
"parent_rev": "17aebfc2104c2df3cd6435fcb09f4085aa4c5ccc", "commit_rev":
"72a4922509a9e32da8ec2706e3b6b6cb46ef340a"}
Message was sent while issue was closed.
Description was changed from ========== Remove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler) Currently, ArrayIncludes handles the hypothetical case of an array with a fast ElementsKind and non-SMI length. This should not happen (and is checked against in JSArray::JSArrayVerify of objects_debug.cc). Therefore this CL replaces that handling with a CSA_ASSERT that the length is indeed SMI. The CL also simplifies loading of the (SMI) length on 64 bit architectures by using LoadAndUntagObjectField instead of LoadObjectField+SmiToWord. BUG=v8:5985 ========== to ========== Remove SMI length check from TF_BUILTIN(ArrayIncludes, CodeStubAssembler) Currently, ArrayIncludes handles the hypothetical case of an array with a fast ElementsKind and non-SMI length. This should not happen (and is checked against in JSArray::JSArrayVerify of objects_debug.cc). Therefore this CL replaces that handling with a CSA_ASSERT that the length is indeed SMI. The CL also simplifies loading of the (SMI) length on 64 bit architectures by using LoadAndUntagObjectField instead of LoadObjectField+SmiToWord. BUG=v8:5985 Review-Url: https://codereview.chromium.org/2714193002 Cr-Commit-Position: refs/heads/master@{#43433} Committed: https://chromium.googlesource.com/v8/v8/+/72a4922509a9e32da8ec2706e3b6b6cb46e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/72a4922509a9e32da8ec2706e3b6b6cb46e...
Message was sent while issue was closed.
Thanks for the tip to remove the braces! Vaclav https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714193002/diff/80001/src/builtins/builtins-a... src/builtins/builtins-array.cc:1701: Node* len = nullptr; On 2017/02/26 18:34:27, Benedikt Meurer wrote: > On 2017/02/25 21:25:55, vabr (Chromium) wrote: > > I tried to initialize |len| with LoadAndUntagObjectField directly, after line > > 1713 (see patch set 4), but RawMachineAssembler::AddNode complained that its > > |current_block_| is null. I'm not sure how that notion of a block corresponds > to > > the structure of the code here, but assigning the result of > > LoadAndUntagObjectField inside the code block on lines 1703-1713 helped. > > Just remove the { and } in 1704, and declare the Node* Len with the assignment > below. Done.
Message was sent while issue was closed.
LGTM! |
