Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714173002:
Remove SMI length check from Builtins::Generate_ArrayIndexOf (Closed)
|
Created:
3 years, 10 months ago by vabr (Chromium) Modified:
3 years, 10 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionRemove SMI length check from Builtins::Generate_ArrayIndexOf
Currently, Generate_ArrayIndexOf 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.
The CL does not add new tests, because test/mjsunit/array-length.js should
cover this already.
BUG=v8:5985
Review-Url: https://codereview.chromium.org/2714173002
Cr-Commit-Position: refs/heads/master@{#43431}
Committed: https://chromium.googlesource.com/v8/v8/+/2a7957b957feb53a073d356a00b0fae5561e1625
Patch Set 1 #Patch Set 2 : Fix comments #
Total comments: 4
Patch Set 3 : Assert on untagged #Patch Set 4 : No 'ToWord32' #Messages
Total messages: 31 (20 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 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...)
vabr@chromium.org changed reviewers: + bmeurer@chromium.org
Hi Benedikt, Could you please check that I'm not doing something stupid in how I load the length and how I formulated the CSA_ASSERT? A number of tests are failing, and I will investigate why exactly (later, need to run home now), but it would help to have a sanity check on the code change. Thanks! Vaclav
caitp@igalia.com changed reviewers: + caitp@igalia.com
https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2027: CSA_ASSERT(&assembler, assembler.TaggedIsSmi(len)); You're doing a LoadAndUntagToWord32ObjectField, which untags the Smi. You'd need to change this to just LoadObjectField() and untag it after the assert, or otherwise load the object field without untagging in the assertion.
https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2027: CSA_ASSERT(&assembler, assembler.TaggedIsSmi(len)); On 2017/02/24 16:09:16, caitp wrote: > You're doing a LoadAndUntagToWord32ObjectField, which untags the Smi. > > You'd need to change this to just LoadObjectField() and untag it after the > assert, or otherwise load the object field without untagging in the assertion. There may not be any point in untagging though, I think you could just modify the entire stub to use Smi comparison and increment operations instead.
https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2027: CSA_ASSERT(&assembler, assembler.TaggedIsSmi(len)); > There may not be any point in untagging though, I think you could just modify > the entire stub to use Smi comparison and increment operations instead. That's less efficient on 64-bit platforms, since the Smis cannot be immediates there. So the fix here is to use: Node* len = LoadAndUntagObjectField(...); CSA_ASSERT(TaggedIsSmi(LoadObjectField(...));
https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... src/builtins/builtins-array.cc:2027: CSA_ASSERT(&assembler, assembler.TaggedIsSmi(len)); On 2017/02/24 17:50:06, Benedikt Meurer wrote: > > There may not be any point in untagging though, I think you could just modify > > the entire stub to use Smi comparison and increment operations instead. > > That's less efficient on 64-bit platforms, since the Smis cannot be immediates > there. So the fix here is to use: > > Node* len = LoadAndUntagObjectField(...); > CSA_ASSERT(TaggedIsSmi(LoadObjectField(...)); Fair enough --- might be worth updating the ArrayIncludes fast path to untag the index/length as well, then
On 2017/02/24 18:19:05, caitp wrote: > https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... > File src/builtins/builtins-array.cc (right): > > https://codereview.chromium.org/2714173002/diff/20001/src/builtins/builtins-a... > src/builtins/builtins-array.cc:2027: CSA_ASSERT(&assembler, > assembler.TaggedIsSmi(len)); > On 2017/02/24 17:50:06, Benedikt Meurer wrote: > > > There may not be any point in untagging though, I think you could just > modify > > > the entire stub to use Smi comparison and increment operations instead. > > > > That's less efficient on 64-bit platforms, since the Smis cannot be immediates > > there. So the fix here is to use: > > > > Node* len = LoadAndUntagObjectField(...); > > CSA_ASSERT(TaggedIsSmi(LoadObjectField(...)); > > Fair enough --- might be worth updating the ArrayIncludes fast path to untag the > index/length as well, then Good point! vabr@ can you do that in a follow-up CL?
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 ========== Remove SMI length check from Builtins::Generate_ArrayIndexOf Currently, Generate_ArrayIndexOf 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. The CL does not add new tests, because test/mjsunit/array-length.js should cover this already. BUG=v8:5985 ========== to ========== Remove SMI length check from Builtins::Generate_ArrayIndexOf Currently, Generate_ArrayIndexOf 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. The CL does not add new tests, because test/mjsunit/array-length.js should cover this already. BUG=v8:5985 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi caitp@ and Benedikt! First of all: thank you, caitp@, for your help here and on the bug. I really appreciated your insight and explanation of holey elements, as well as making me aware of the tagging issues here. Also thanks to Benedikt, for his offline consultations and patience with me. I followed your joint guidance here and the bots seem to be happy now. Please have a look. The CL for ArrayIncludes is https://codereview.chromium.org/2714193002/. I wrote it yesterday, with the same issues as this one. Your advice here also applied to there, so it's ready for review as well. Cheers, Vaclav P.S. The v8_linux64_verify_csa_rel_ng seems to have some clever static analysis, because it spotted the CSA_ASSERT issue already during compilation. Do you have any idea how to set up my build like that?
Perfect, LGTM. For the CSA verification, you need to set the GN flag v8_enable_verify_csa to true (ideally in a Debug build).
On 2017/02/25 17:55:29, Benedikt Meurer wrote: > Perfect, LGTM. > > For the CSA verification, you need to set the GN flag v8_enable_verify_csa to > true (ideally in a Debug build). Thanks, Benedikt! (Also for telling me the GN flag, that will be useful.) caitp@ -- I will land now, because Benedikt already approved the CL. But please feel free to leave any further comments which you might have. I will be more than happy to do a follow-up and learn more about V8 from you! Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
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": 1488045772506370,
"parent_rev": "d126e3fc5561a774cf5e4c70441ac585b2131e74", "commit_rev":
"2a7957b957feb53a073d356a00b0fae5561e1625"}
Message was sent while issue was closed.
Description was changed from ========== Remove SMI length check from Builtins::Generate_ArrayIndexOf Currently, Generate_ArrayIndexOf 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. The CL does not add new tests, because test/mjsunit/array-length.js should cover this already. BUG=v8:5985 ========== to ========== Remove SMI length check from Builtins::Generate_ArrayIndexOf Currently, Generate_ArrayIndexOf 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. The CL does not add new tests, because test/mjsunit/array-length.js should cover this already. BUG=v8:5985 Review-Url: https://codereview.chromium.org/2714173002 Cr-Commit-Position: refs/heads/master@{#43431} Committed: https://chromium.googlesource.com/v8/v8/+/2a7957b957feb53a073d356a00b0fae5561... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/2a7957b957feb53a073d356a00b0fae5561... |
