[regexp] Port RegExp.prototype[@@replace]

[regexp] Port RegExp.prototype[@@replace]

This moves the implementation of @@replace from regexp.js to builtins-regexp.cc
(the TurboFan fast path) and runtime-regexp.cc (slow path). The fast path
handles all cases in which the regexp itself is an unmodified JSRegExp
instance, the given 'replace' argument is not callable and does not contain any
'$' characters (i.e. we are doing a string replacement).

BUG=v8:5339
Committed: https://crrev.com/33a4faa47d664328464cb11597ac36d07051d7dd
Cr-Commit-Position: refs/heads/master@{#40253}

[jgruber, 2016-10-07 08:45:20]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 08:45:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 09:20:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 09:20:38]

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/14110)
  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...)

[jgruber, 2016-10-07 13:30:15]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 13:30:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 13:31:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 13:31:47]

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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14167)
  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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14128)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10370)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25938)

[jgruber, 2016-10-07 14:19:40]

jgruber@chromium.org changed reviewers:
+ yangguo@chromium.org

[jgruber, 2016-10-07 14:19:41]

This is the largest remaining part of the regexp port. We optimize for simple
string replacement (right now we naively check that the replacement string does
not have any '$' characters). Non-global simple string replacement has a
TurboFan fast path, and global simple string replacement goes through runtime as
before.

[Yang, 2016-10-11 13:26:35]

lgtm

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:554: a->WordEqual(proto_map,
initial_proto_initial_map);
If we modify a property on the regexp prototype, does its map change? Can we
have a test case?

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1497: Node* const has_initialmap =
a->WordEqual(int_zero, int_zero);  // True.
This would be the same as a->TrueConstant. Let's instead just not emit the
branch in StoreLastIndex at all.

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
File src/runtime/runtime-regexp.cc (right):

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1111: Handle<FixedArray> result_array =
factory->NewFixedArrayWithHoles(16);
I wonder whether we can't simply use a List<Handle<Object>> for result_array.

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1136: if (elem->IsSmi()) {
Maybe we don't need this weird encoding anymore (in preparation to invoking
StringBuilderConcat), but can just copy into the result string with the
IncrementalStringBuilder as we go?

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1183: // for result_array?
Yes. We could either merge the loop in SearchRegExpMultiple (to find all
matches) with this loop (to create the results), or we could use an off-heap
data structure instead of JSArrays to pass each match.

[jgruber, 2016-10-13 07:39:34]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 07:39:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-10-13 07:39:44]

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:554: a->WordEqual(proto_map,
initial_proto_initial_map);
On 2016/10/11 13:26:35, Yang wrote:
> If we modify a property on the regexp prototype, does its map change? Can we
> have a test case?

Added.

https://codereview.chromium.org/2398423002/diff/30001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1497: Node* const has_initialmap =
a->WordEqual(int_zero, int_zero);  // True.
On 2016/10/11 13:26:35, Yang wrote:
> This would be the same as a->TrueConstant. Let's instead just not emit the
> branch in StoreLastIndex at all.

Done.

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
File src/runtime/runtime-regexp.cc (right):

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1111: Handle<FixedArray> result_array =
factory->NewFixedArrayWithHoles(16);
On 2016/10/11 13:26:35, Yang wrote:
> I wonder whether we can't simply use a List<Handle<Object>> for result_array.

Yes, plenty of potential for optimization in these functions. I'd like to follow
up with this in a later CL though (this is also not a path that octane/regexp
takes). Updated the TODO text above to reflect our intentions.

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1136: if (elem->IsSmi()) {
On 2016/10/11 13:26:35, Yang wrote:
> Maybe we don't need this weird encoding anymore (in preparation to invoking
> StringBuilderConcat), but can just copy into the result string with the
> IncrementalStringBuilder as we go?

Yes, it'd be great to make this more readable. Added a TODO.

https://codereview.chromium.org/2398423002/diff/30001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1183: // for result_array?
On 2016/10/11 13:26:35, Yang wrote:
> Yes. We could either merge the loop in SearchRegExpMultiple (to find all
> matches) with this loop (to create the results), or we could use an off-heap
> data structure instead of JSArrays to pass each match.

Likewise, updated the TODO.

[commit-bot: I haz the power, 2016-10-13 08:09:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 08:09:46]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-10-13 10:28:41]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 10:28:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 10:43:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 10:43:35]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Igor Sheludko, 2016-10-13 10:45:16]

ishell@chromium.org changed reviewers:
+ ishell@chromium.org

[Igor Sheludko, 2016-10-13 10:45:17]

lgtm with nits:

https://codereview.chromium.org/2398423002/diff/70001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2398423002/diff/70001/src/code-stub-assembler...
src/code-stub-assembler.cc:2555: Node* const smi_zero =
a->SmiConstant(Smi::FromInt(0));
Smi::kZero, here and below.

[jgruber, 2016-10-13 10:47:21]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 10:47:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-10-13 10:49:05]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 10:49:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-10-13 10:50:48]

Thanks!

https://codereview.chromium.org/2398423002/diff/70001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2398423002/diff/70001/src/code-stub-assembler...
src/code-stub-assembler.cc:2555: Node* const smi_zero =
a->SmiConstant(Smi::FromInt(0));
On 2016/10/13 10:45:16, Igor Sheludko wrote:
> Smi::kZero, here and below.

Done.

[commit-bot: I haz the power, 2016-10-13 11:19:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 11:19:29]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-10-13 11:25:04]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-10-13 11:25:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2398423002/#ps110001
(title: "Smi::kZero")

[commit-bot: I haz the power, 2016-10-13 11:25:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 11:27:31]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-10-13 11:27:54]

Description was changed from

==========
[regexp] Port RegExp.prototype[@@replace]

This moves the implementation of @@replace from regexp.js to builtins-regexp.cc
(the TurboFan fast path) and runtime-regexp.cc (slow path). The fast path
handles all cases in which the regexp itself is an unmodified JSRegExp
instance, the given 'replace' argument is not callable and does not contain any
'$' characters (i.e. we are doing a string replacement).

BUG=v8:5339
==========

to

==========
[regexp] Port RegExp.prototype[@@replace]

This moves the implementation of @@replace from regexp.js to builtins-regexp.cc
(the TurboFan fast path) and runtime-regexp.cc (slow path). The fast path
handles all cases in which the regexp itself is an unmodified JSRegExp
instance, the given 'replace' argument is not callable and does not contain any
'$' characters (i.e. we are doing a string replacement).

BUG=v8:5339

Committed: https://crrev.com/33a4faa47d664328464cb11597ac36d07051d7dd
Cr-Commit-Position: refs/heads/master@{#40253}
==========

[commit-bot: I haz the power, 2016-10-13 11:27:55]

Patchset 7 (id:??) landed as
https://crrev.com/33a4faa47d664328464cb11597ac36d07051d7dd
Cr-Commit-Position: refs/heads/master@{#40253}

[zhengxing.li, 2016-10-14 15:22:11]

On 2016/10/13 11:27:55, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/33a4faa47d664328464cb11597ac36d07051d7dd
> Cr-Commit-Position: refs/heads/master@{#40253}

Hi, all:

This CL caused a gcc compilation error.

For example: 
1. switch to gcc compiler
2. make ia32.optdebug snapsho=off -jn

then you got the following error message:
**********************************************************************************************************************************************************
In file included from .././src/base/base-export.h:8:0,
                 from .././src/base/logging.h:12,
                 from .././src/runtime/runtime-utils.h:8,
                 from ../src/runtime/runtime-regexp.cc:5:
.././src/vector.h: In function ‘v8::internal::Object*
v8::internal::__RT_impl_Runtime_RegExpReplace(v8::internal::Arguments,
v8::internal::Isolate*)’:
.././include/v8config.h:357:67: error: assuming signed overflow does not occur
when assuming that (X + c) < X is always false [-Werror=strict-overflow]
 # define V8_UNLIKELY(condition) (__builtin_expect(!!(condition), 0))
                                                                   ^
.././src/base/logging.h:48:9: note: in expansion of macro ‘V8_UNLIKELY’
     if (V8_UNLIKELY(!(condition))) {                                 \
         ^
.././src/base/logging.h:156:32: note: in expansion of macro ‘CHECK’
 #define DCHECK(condition)      CHECK(condition)
                                ^
.././src/vector.h:54:5: note: in expansion of macro ‘DCHECK’
     DCHECK(0 <= index && index < length_);
     ^
  CXX(target)
/home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/wasm/wasm-module.o
cc1plus: all warnings being treated as errors
make[1]: ***
[/home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/runtime/runtime-regexp.o]
Error 1
**********************************************************************************************************************************************************

Would you please take a look at this issue?

Thanks!