[Interpreter] Add implementations for load immediate bytecodes.

[Interpreter] Add implementations for load immediate bytecodes.

Adds implementations and tests for the following bytecodes:
  - LdaZero
  - LdaSmi8
  - LdaUndefined
  - LdaNull
  - LdaTheHole
  - LdaTrue
  - LdaFalse
  - LdaLdar
  - LdaStar

Also adds  Smi tagging / untagging and OperandType typed
BytecodeOperand operations to InterpreterAssembler.

BUG=v8:4280
LOG=N

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

[rmcilroy, 2015-08-14 14:34:03]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, oth@chromium.org

[rmcilroy, 2015-08-14 14:34:03]

Michael, Orion, could you take a look please? Thanks.

[rmcilroy, 2015-08-14 14:44:51]

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

[commit-bot: I haz the power, 2015-08-14 14:44:57]

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

[rmcilroy, 2015-08-14 14:45:19]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-08-14 14:45:51]

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

[commit-bot: I haz the power, 2015-08-14 14:45:57]

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

[commit-bot: I haz the power, 2015-08-14 15:11:22]

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

[commit-bot: I haz the power, 2015-08-14 15:11:22]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-08-17 08:22:34]

LGTM modulo one comment about evaluation order.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:39: // Returns the Imm8 immediate for
bytecode operand |index| in the current
nit: Comment seems to be outdated, no index is passed in.

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:149: __ StoreRegister(__ GetAccumulator(), __
BytecodeOperandReg(0));
This pattern looks dangerous, note that C++ does not specify the evaluation
order or the argument, so here either "GetAccumulator" or "BytecodeOperandReg"
could be assembled first into the bytecode stream.

[Michael Starzinger, 2015-08-17 08:27:45]

To clarify my comment: I think we should put sequence points between all
invocations of the bytecode assembler, not just the ones with more than one
argument. This feels like asking for trouble.

[oth, 2015-08-17 10:49:02]

A couple of minor comments and modulo Michael's comment looks good here.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:137: Node*
InterpreterAssembler::BytecodeOperandImm8(int delta) {
Suggest renaming delta for clarity - operand_index/operand_number.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:5: #ifndef
V8_COMPILER_INTERPRETER_CODEGEN_H_
Aaron Gray correctly highlighted the need for:
s/INTERPRETER_CODEGEN/INTERPRETER_ASSEMBLER/

[rmcilroy, 2015-08-18 13:06:22]

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:137: Node*
InterpreterAssembler::BytecodeOperandImm8(int delta) {
On 2015/08/17 10:49:02, oth wrote:
> Suggest renaming delta for clarity - operand_index/operand_number.

Done.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:5: #ifndef
V8_COMPILER_INTERPRETER_CODEGEN_H_
On 2015/08/17 10:49:02, oth wrote:
> Aaron Gray correctly highlighted the need for:
> s/INTERPRETER_CODEGEN/INTERPRETER_ASSEMBLER/
> 

Done.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:39: // Returns the Imm8 immediate for
bytecode operand |index| in the current
On 2015/08/17 08:22:34, Michael Starzinger wrote:
> nit: Comment seems to be outdated, no index is passed in.

Changed parameter and comment to operand_index

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:149: __ StoreRegister(__ GetAccumulator(), __
BytecodeOperandReg(0));
On 2015/08/17 08:22:34, Michael Starzinger wrote:
> This pattern looks dangerous, note that C++ does not specify the evaluation
> order or the argument, so here either "GetAccumulator" or "BytecodeOperandReg"
> could be assembled first into the bytecode stream.

Yeah good point. Note, in this case it doesn't matter for correctness which
argument is evaluated first, but agree that this could be confusing and
potentially dangerous later. Fixed to define the order.

[rmcilroy, 2015-08-18 13:08:54]

Patchset #2 (id:40001) has been deleted

[rmcilroy, 2015-08-18 13:33:24]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-08-18 13:33:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1294793002/#ps60001
(title: "Address review comments.")

[commit-bot: I haz the power, 2015-08-18 13:33:37]

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

[commit-bot: I haz the power, 2015-08-18 13:58:56]

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

[commit-bot: I haz the power, 2015-08-18 13:58:57]

Failed to apply patch for src/interpreter/bytecode-array-builder.cc:
While running git apply --index -3 -p1;
  error: patch failed: src/interpreter/bytecode-array-builder.cc:50
  Falling back to three-way merge...
  Applied patch to 'src/interpreter/bytecode-array-builder.cc' with conflicts.
  U src/interpreter/bytecode-array-builder.cc

Patch:       src/interpreter/bytecode-array-builder.cc
Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc
b/src/interpreter/bytecode-array-builder.cc
index
8cc0772db97d4b0a98180c57d5c0ffa28d8cc726..04e970101c90bc1365b217c1e9a388bc72defbcc
100644
--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -50,7 +50,7 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::LoadLiteral(
   int32_t raw_smi = smi->value();
   if (raw_smi == 0) {
     Output(Bytecode::kLdaZero);
-  } else if (raw_smi > -128 && raw_smi <= 128) {
+  } else if (raw_smi >= -128 && raw_smi < 128) {
     Output(Bytecode::kLdaSmi8, static_cast<uint8_t>(raw_smi));
   } else {
     // TODO(oth): Put Smi in constant pool.

[commit-bot: I haz the power, 2015-08-18 13:58:58]

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

[rmcilroy, 2015-08-18 14:58:31]

Patchset #3 (id:80001) has been deleted

[rmcilroy, 2015-08-18 15:04:56]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-08-18 15:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1294793002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-18 15:05:03]

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

[commit-bot: I haz the power, 2015-08-18 15:29:27]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-08-18 15:29:46]

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