CodeStubAssembler can generate code for builtins

CodeStubAssembler can generate code for builtins

This will be used for generating the Atomics builtins.

BUG=v8:4614
R=jarin@chromium.org
LOG=y

Committed: https://crrev.com/b9aa3ce764b06d2fc2273e0bba95ae373b722281
Cr-Commit-Position: refs/heads/master@{#34413}

[binji, 2016-02-18 02:22:30]

PTAL.

This was extracted from https://codereview.chromium.org/1617503003/.

The test is not working, but I don't understand why. The generated code is:

   0x149fc3140f60:	push   %rbp
   0x149fc3140f61:	mov    %rsp,%rbp
   0x149fc3140f64:	push   %rsi
   0x149fc3140f65:	push   %rdi
   0x149fc3140f66:	mov    0x18(%rbp),%rax
   0x149fc3140f6a:	sar    $0x20,%rax
   0x149fc3140f6e:	mov    0x10(%rbp),%rbx
   0x149fc3140f72:	sar    $0x20,%rbx
   0x149fc3140f76:	lea    (%rax,%rbx,1),%eax
   0x149fc3140f79:	shl    $0x20,%rax
   0x149fc3140f7d:	mov    %rbp,%rsp
   0x149fc3140f80:	pop    %rbp
   0x149fc3140f81:	retq   $0x18

At the beginning of the function 0x18(%rbp) and 0x10(%rbp) have the correct
values. But after %rbp is updated, the values are wrong. I know I'm setting up
something wrong, but I'm not sure what. :)

[Jarin, 2016-02-18 09:04:39]

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
File test/cctest/compiler/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
test/cctest/compiler/test-code-stub-assembler.cc:264: FunctionTester
ft(kNumParams - 1, code);  // Implicit undefined receiver.
Apparently, there is a bug in FunctionTester::BuildFunction - it adds an extra
argument to the function. Sorry about that.

Fix here: https://codereview.chromium.org/1711513003/ (will try to land ASAP).

[Jarin, 2016-02-18 10:39:06]

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

[commit-bot: I haz the power, 2016-02-18 10:39:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705073005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705073005/1

[commit-bot: I haz the power, 2016-02-18 11:18:09]

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

[commit-bot: I haz the power, 2016-02-18 11:18:10]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[Jarin, 2016-02-18 12:32:15]

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
File test/cctest/compiler/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
test/cctest/compiler/test-code-stub-assembler.cc:269: }
Actually, FunctionTester as a whole seems to be a terrible hack (this was
pointed out to me by Benedikt, I agree). How about this:

namespace {

Handle<JSFunction> CreateFunctionFromCode(int parameter_count_with_receiver,
                                          Handle<Code> code) {
  Isolate* isolate = code->GetIsolate();
  Handle<String> name = isolate->factory()->InternalizeUtf8String("test");
  Handle<JSFunction> function =
      isolate->factory()->NewFunctionWithoutPrototype(name, code);
  function->shared()->set_internal_formal_parameter_count(
      parameter_count_with_receiver - 1);  // Implicit undefined receiver.
  return function;
}

}  // namespace

TEST(JSFunction) {
  const int kNumParams = 3;  // Receiver, left, right.
  Isolate* isolate(CcTest::InitIsolateOnce());
  CodeStubAssemblerTester m(isolate, kNumParams);
  m.Return(m.SmiTag(
      m.Int32Add(m.SmiUntag(m.Parameter(1)), m.SmiUntag(m.Parameter(2)))));
  Handle<Code> code = m.GenerateCode();
  Handle<JSFunction> function = CreateFunctionFromCode(kNumParams, code);

  // Run the function.
  Handle<Object> args[] = {Handle<Smi>(Smi::FromInt(23), isolate),
                           Handle<Smi>(Smi::FromInt(34), isolate)};
  MaybeHandle<Object> result =
      Execution::Call(isolate, function, isolate->factory()->undefined_value(),
                      arraysize(args), args);
  CHECK_EQ(57, Handle<Smi>::cast(result.ToHandleChecked())->value());
}

[binji, 2016-02-18 19:35:18]

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
File test/cctest/compiler/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
test/cctest/compiler/test-code-stub-assembler.cc:264: FunctionTester
ft(kNumParams - 1, code);  // Implicit undefined receiver.
On 2016/02/18 09:04:38, Jarin wrote:
> Apparently, there is a bug in FunctionTester::BuildFunction - it adds an extra
> argument to the function. Sorry about that.
> 
> Fix here: https://codereview.chromium.org/1711513003/ (will try to land ASAP).

Acknowledged.

https://codereview.chromium.org/1705073005/diff/1/test/cctest/compiler/test-c...
test/cctest/compiler/test-code-stub-assembler.cc:269: }
On 2016/02/18 12:32:15, Jarin wrote:
> Actually, FunctionTester as a whole seems to be a terrible hack (this was
> pointed out to me by Benedikt, I agree). How about this:
> 
> namespace {
> 
> Handle<JSFunction> CreateFunctionFromCode(int parameter_count_with_receiver,
>                                           Handle<Code> code) {
>   Isolate* isolate = code->GetIsolate();
>   Handle<String> name = isolate->factory()->InternalizeUtf8String("test");
>   Handle<JSFunction> function =
>       isolate->factory()->NewFunctionWithoutPrototype(name, code);
>   function->shared()->set_internal_formal_parameter_count(
>       parameter_count_with_receiver - 1);  // Implicit undefined receiver.
>   return function;
> }
> 
> }  // namespace
> 
> TEST(JSFunction) {
>   const int kNumParams = 3;  // Receiver, left, right.
>   Isolate* isolate(CcTest::InitIsolateOnce());
>   CodeStubAssemblerTester m(isolate, kNumParams);
>   m.Return(m.SmiTag(
>       m.Int32Add(m.SmiUntag(m.Parameter(1)), m.SmiUntag(m.Parameter(2)))));
>   Handle<Code> code = m.GenerateCode();
>   Handle<JSFunction> function = CreateFunctionFromCode(kNumParams, code);
> 
>   // Run the function.
>   Handle<Object> args[] = {Handle<Smi>(Smi::FromInt(23), isolate),
>                            Handle<Smi>(Smi::FromInt(34), isolate)};
>   MaybeHandle<Object> result =
>       Execution::Call(isolate, function,
isolate->factory()->undefined_value(),
>                       arraysize(args), args);
>   CHECK_EQ(57, Handle<Smi>::cast(result.ToHandleChecked())->value());
> }
> 

Done.

[binji, 2016-02-18 19:35:31]

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

[commit-bot: I haz the power, 2016-02-18 19:35:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705073005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705073005/20001

[commit-bot: I haz the power, 2016-02-18 19:52:24]

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

[commit-bot: I haz the power, 2016-02-18 19:52:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[binji, 2016-02-18 20:27:44]

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

[commit-bot: I haz the power, 2016-02-18 20:27:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705073005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705073005/40001

[commit-bot: I haz the power, 2016-02-18 20:49:41]

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

[commit-bot: I haz the power, 2016-02-18 20:49:43]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/3029)
  v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[binji, 2016-02-18 23:28:20]

Weird, looks like the rounding mode for ChangeFloat64ToUint32 is different on
x86 vs. x64?

[binji, 2016-02-18 23:42:30]

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

[commit-bot: I haz the power, 2016-02-18 23:42:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705073005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705073005/60001

[commit-bot: I haz the power, 2016-02-19 00:20:01]

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

[commit-bot: I haz the power, 2016-02-19 00:20:03]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[binji, 2016-02-19 03:34:48]

On 2016/02/19 00:20:03, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

Not sure what's up with the arm64 bot, but I can repro the error locally too.

[Jarin, 2016-02-19 06:55:29]

On 2016/02/19 03:34:48, binji wrote:
> On 2016/02/19 00:20:03, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
> 
> Not sure what's up with the arm64 bot, but I can repro the error locally too.

Not sure either, otherwise the CL looks great.

[Jarin, 2016-02-19 06:55:34]

On 2016/02/19 03:34:48, binji wrote:
> On 2016/02/19 00:20:03, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
> 
> Not sure what's up with the arm64 bot, but I can repro the error locally too.

Not sure either, otherwise the CL looks great.

[Jarin, 2016-02-19 07:15:54]

On 2016/02/18 23:28:20, binji wrote:
> Weird, looks like the rounding mode for ChangeFloat64ToUint32 is different on
> x86 vs. x64?

Generally, the Change* operations only work if the input is in the target range,
i.e., in this case the input must be an integer in [0 .. 2^32-1]. If the value
is outside that range, the result is undefined (I think the difference between
arm and x86 is even bigger - on one platform we clamp, on the other we just use
the low 32 bits).

If your value is not inside the range, you should call Truncate* operations.

[binji, 2016-02-19 18:44:35]

On 2016/02/19 07:15:54, Jarin wrote:
> On 2016/02/18 23:28:20, binji wrote:
> > Weird, looks like the rounding mode for ChangeFloat64ToUint32 is different
on
> > x86 vs. x64?
> 
> Generally, the Change* operations only work if the input is in the target
range,
> i.e., in this case the input must be an integer in [0 .. 2^32-1]. If the value
> is outside that range, the result is undefined (I think the difference between
> arm and x86 is even bigger - on one platform we clamp, on the other we just
use
> the low 32 bits).
> 
> If your value is not inside the range, you should call Truncate* operations.

Ah, that makes sense. Thanks!

[Jarin, 2016-02-29 20:04:43]

jarin@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Jarin, 2016-02-29 20:04:43]

lgtm from my side, but Benedikt had some plans with builtin code generators, so
I am adding him as a reviewer.

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:243: Node* InstanceType(Node* object);
InstanceType -> LoadInstanceType?

[Benedikt Meurer, 2016-03-01 04:54:45]

Nice, just a few naming nits. Please go ahead, LGTM!

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:219: Node* SmiUntag32(Node* value);
Nit: Rename to SmiToInt32

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:236: Node* LoadHeapNumber(Node* object);
Nit: Rename to LoadHeapNumberValue

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:247: Node* BitFieldValue(Node* word32) {
Nit: Rename to BitFieldDecode

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:251: Node* BitFieldValue(Node* word32,
uint32_t shift, uint32_t mask);
Nit: Rename to BitFieldDecode

[binji, 2016-03-01 21:55:28]

The CQ bit was checked by binji@chromium.org

[binji, 2016-03-01 21:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1705073005/#ps140001
(title: "rename")

[commit-bot: I haz the power, 2016-03-01 21:55:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705073005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705073005/140001

[binji, 2016-03-01 21:56:38]

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:219: Node* SmiUntag32(Node* value);
On 2016/03/01 04:54:44, Benedikt Meurer wrote:
> Nit: Rename to SmiToInt32

Done.

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:236: Node* LoadHeapNumber(Node* object);
On 2016/03/01 04:54:44, Benedikt Meurer wrote:
> Nit: Rename to LoadHeapNumberValue

Done.

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:243: Node* InstanceType(Node* object);
On 2016/02/29 20:04:43, Jarin (OOO vacation) wrote:
> InstanceType -> LoadInstanceType?

Done.

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:247: Node* BitFieldValue(Node* word32) {
On 2016/03/01 04:54:44, Benedikt Meurer wrote:
> Nit: Rename to BitFieldDecode

Done.

https://codereview.chromium.org/1705073005/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:251: Node* BitFieldValue(Node* word32,
uint32_t shift, uint32_t mask);
On 2016/03/01 04:54:44, Benedikt Meurer wrote:
> Nit: Rename to BitFieldDecode

Done.

[commit-bot: I haz the power, 2016-03-01 22:05:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-01 22:05:40]

Description was changed from

==========
CodeStubAssembler can generate code for builtins

This will be used for generating the Atomics builtins.

BUG=v8:4614
R=jarin@chromium.org
LOG=y
==========

to

==========
CodeStubAssembler can generate code for builtins

This will be used for generating the Atomics builtins.

BUG=v8:4614
R=jarin@chromium.org
LOG=y

Committed: https://crrev.com/b9aa3ce764b06d2fc2273e0bba95ae373b722281
Cr-Commit-Position: refs/heads/master@{#34413}
==========

[commit-bot: I haz the power, 2016-03-01 22:05:43]

Patchset 8 (id:??) landed as
https://crrev.com/b9aa3ce764b06d2fc2273e0bba95ae373b722281
Cr-Commit-Position: refs/heads/master@{#34413}