Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407813002:
[stubs] Port StringAddStub to TF (Closed)
Description[stubs] Port StringAddStub to TF
In the process:
- Add ToString to the CodeStubAssembler and use it where appropriate
- Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code
in element offset computation, especially for strings.
BUG=chromium:608675
LOG=N
Committed: https://crrev.com/787157dd0f9de2668ff0ae162405a3e191851d6e
Cr-Commit-Position: refs/heads/master@{#40379}
Patch Set 1 #Patch Set 2 : It works #Patch Set 3 : Left/right convert #Patch Set 4 : Latest #Patch Set 5 : Merge with ToT #Patch Set 6 : Merge with ToT #Patch Set 7 : Simplify code #Patch Set 8 : Fix write barrier problems and rebase #Patch Set 9 : Cleanup #Patch Set 10 : Fix counters #Patch Set 11 : Rebase #Patch Set 12 : Remove stray change #
Total comments: 24
Patch Set 13 : Review feedback #Patch Set 14 : Review feedback #Patch Set 15 : Fix test and stuff #Patch Set 16 : Latest changes and feedback #Patch Set 17 : Cleanup #Patch Set 18 : Fix constant handling in string allocation #Patch Set 19 : Fix Smi constant handling #Patch Set 20 : Remove printf #
Total comments: 1
Patch Set 21 : Make ASSERT cheaper #
Messages
Total messages: 74 (64 generated)
The CQ bit was checked by danno@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 danno@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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14473) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14489) 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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14437) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10695) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26348)
The CQ bit was checked by danno@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 ========== [stubs] Port StringAddStub to TF ========== to ========== [stubs] Port StringAddStub to TF In the process: - Add ToString to the CodeStubAssembler and use it where appropriate - Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code in element offset computation, especially for strings. BUG=chromium:608675 LOG=N ==========
The CQ bit was checked by danno@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...
danno@chromium.org changed reviewers: + bmeurer@chromium.org, ishell@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14443) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by danno@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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10735)
The CQ bit was checked by danno@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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10737)
The CQ bit was checked by danno@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 danno@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...
danno@chromium.org changed reviewers: + jgruber@chromium.org
Please take another look
LGTM with comments https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... File src/builtins/builtins-conversion.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... src/builtins/builtins-conversion.cc:186: { assembler->Return(assembler->ToString(context, input)); } Shouldn't this be NumberToString instead of ToString? Also the brackets could be removed. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3412: LoadRoot(Heap::kConsOneByteStringMapRootIndex)); Would it make sense to reuse Allocate{One,Two}ByteConsString? https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3439: Node* CodeStubAssembler::StringAdd(Node* context, Node* left, Node* right, This is basically a more complete version of StringConcat, right? Awesome, that helps RegExp.proto.replace. StringConcat should just be removed. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.h:447: // |from_index|, |to_index| and |character_count| must be Smis s.t. Comment should be updated (or removed) to reflect ParameterMode (previously it just assumed smi parameters).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits: https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... File src/builtins/builtins-conversion.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... src/builtins/builtins-conversion.cc:186: { assembler->Return(assembler->ToString(context, input)); } I guess you intended to call assembler->NumberToString() here. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3389: // both strings, or I wonder why does kOneByteStringTag not imply kOneByteDataHintTag? https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3393: STATIC_ASSERT(kOneByteDataHintMask != 0); kOneByteDataHintTag != 0 https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3404: IntPtrConstant(kOneByteStringTag | kOneByteDataHintMask)), kOneByteStringTag -> kStringEncodingMask https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3464: GotoIf(IntPtrGreaterThan(new_length, IntPtrConstant(String::kMaxLength)), How about doing one unsigned comparison instead of two signed? https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3496: IntPtrConstant(0)), 0 -> kTwoByteStringTag or STATIC_ASSERT https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc#newc... src/code-stubs.cc:357: assembler->JSReceiverToPrimitive(context, left)); Note: we could generate a better code here if we combine these two calls (no duplicate smi checks, no duplicate instance type loads in certain cases). https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc#newc... src/code-stubs.cc:373: assembler->TailCallStub(descriptor, assembler->HeapConstant(stub.GetCode()), I think Callable callable = CodeFactory::StringAdd(). assembler->TailCallStub(callable, ...) is better because the correct call interface descriptor will be used automatically.
The CQ bit was checked by danno@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 danno@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14554) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by danno@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 danno@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 danno@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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16227)
The CQ bit was checked by danno@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...
danno@chromium.org changed reviewers: + mstarzinger@chromium.org
Adding Michi for compliler/ directory changes https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... File src/builtins/builtins-conversion.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... src/builtins/builtins-conversion.cc:186: { assembler->Return(assembler->ToString(context, input)); } On 2016/10/13 14:10:16, jgruber wrote: > Shouldn't this be NumberToString instead of ToString? Also the brackets could be > removed. Yes! Good catch. https://codereview.chromium.org/2407813002/diff/220001/src/builtins/builtins-... src/builtins/builtins-conversion.cc:186: { assembler->Return(assembler->ToString(context, input)); } On 2016/10/13 15:41:33, Igor Sheludko wrote: > I guess you intended to call assembler->NumberToString() here. Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3389: // both strings, or On 2016/10/13 15:41:33, Igor Sheludko wrote: > I wonder why does kOneByteStringTag not imply kOneByteDataHintTag? Good question, although before pulling on this string, I just translated the Hydrogen code for correctness. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3393: STATIC_ASSERT(kOneByteDataHintMask != 0); On 2016/10/13 15:41:33, Igor Sheludko wrote: > kOneByteDataHintTag != 0 Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3404: IntPtrConstant(kOneByteStringTag | kOneByteDataHintMask)), On 2016/10/13 15:41:33, Igor Sheludko wrote: > kOneByteStringTag -> kStringEncodingMask Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3412: LoadRoot(Heap::kConsOneByteStringMapRootIndex)); On 2016/10/13 14:10:16, jgruber wrote: > Would it make sense to reuse Allocate{One,Two}ByteConsString? Yes! Good catch! https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3439: Node* CodeStubAssembler::StringAdd(Node* context, Node* left, Node* right, On 2016/10/13 14:10:16, jgruber wrote: > This is basically a more complete version of StringConcat, right? Awesome, that > helps RegExp.proto.replace. StringConcat should just be removed. I totally missed that this existed when I ported from Crankshaft/Hydrogen. Also a good idea! https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3464: GotoIf(IntPtrGreaterThan(new_length, IntPtrConstant(String::kMaxLength)), On 2016/10/13 15:41:33, Igor Sheludko wrote: > How about doing one unsigned comparison instead of two signed? Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.cc:3496: IntPtrConstant(0)), On 2016/10/13 15:41:33, Igor Sheludko wrote: > 0 -> kTwoByteStringTag or STATIC_ASSERT Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stub-assemble... src/code-stub-assembler.h:447: // |from_index|, |to_index| and |character_count| must be Smis s.t. On 2016/10/13 14:10:16, jgruber wrote: > Comment should be updated (or removed) to reflect ParameterMode (previously it > just assumed smi parameters). Done. https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc#newc... src/code-stubs.cc:357: assembler->JSReceiverToPrimitive(context, left)); On 2016/10/13 15:41:33, Igor Sheludko wrote: > Note: we could generate a better code here if we combine these two calls (no > duplicate smi checks, no duplicate instance type loads in certain cases). Yes, but it somewhat breaks the abstraction of having nicely encapulated chunks of functionality. How about I add a TODO and we can consider this if there is a big performance regression or it becomes an issue at some point. https://codereview.chromium.org/2407813002/diff/220001/src/code-stubs.cc#newc... src/code-stubs.cc:373: assembler->TailCallStub(descriptor, assembler->HeapConstant(stub.GetCode()), On 2016/10/13 15:41:33, Igor Sheludko wrote: > I think > Callable callable = CodeFactory::StringAdd(). > assembler->TailCallStub(callable, ...) > is better because the correct call interface descriptor will be used > automatically. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by danno@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...
LGTM on "compiler" with one comment. Didn't look at the rest. https://codereview.chromium.org/2407813002/diff/370001/src/compiler/code-asse... File src/compiler/code-assembler.cc (right): https://codereview.chromium.org/2407813002/diff/370001/src/compiler/code-asse... src/compiler/code-assembler.cc:159: out_value = reinterpret_cast<Smi*>(m.Value()); nit: At this point we should be able to use the checked Smi::cast method for additional safety instead AFAICT. Something along the lines of "Smi::cast(reinterpret_cast<Object*>(m.Value()))" I think. Also not sure whether "reinterpret_cast" or "bit_cast" is the correct thing here.
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 danno@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 danno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, jgruber@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2407813002/#ps390001 (title: "Make ASSERT cheaper")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Feedback addressed.
Message was sent while issue was closed.
Description was changed from ========== [stubs] Port StringAddStub to TF In the process: - Add ToString to the CodeStubAssembler and use it where appropriate - Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code in element offset computation, especially for strings. BUG=chromium:608675 LOG=N ========== to ========== [stubs] Port StringAddStub to TF In the process: - Add ToString to the CodeStubAssembler and use it where appropriate - Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code in element offset computation, especially for strings. BUG=chromium:608675 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #21 (id:390001)
Message was sent while issue was closed.
Description was changed from ========== [stubs] Port StringAddStub to TF In the process: - Add ToString to the CodeStubAssembler and use it where appropriate - Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code in element offset computation, especially for strings. BUG=chromium:608675 LOG=N ========== to ========== [stubs] Port StringAddStub to TF In the process: - Add ToString to the CodeStubAssembler and use it where appropriate - Add constant-folding versions of IntPtrAdd/IntPtrSub to simplify code in element offset computation, especially for strings. BUG=chromium:608675 LOG=N Committed: https://crrev.com/787157dd0f9de2668ff0ae162405a3e191851d6e Cr-Commit-Position: refs/heads/master@{#40379} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/787157dd0f9de2668ff0ae162405a3e191851d6e Cr-Commit-Position: refs/heads/master@{#40379} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
