[turbofan] store block id with instruction

[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the
improvement.

BUG=

Committed: https://crrev.com/81c965d475a51bb9a8df4c4035ccc3ddae1c8980
Cr-Commit-Position: refs/heads/master@{#35711}

[Mircea Trofin, 2016-04-19 05:15:44]

Description was changed from

==========
store block id with instruction

BUG=
==========

to

==========
[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the 
improvement.

BUG=
==========

[Mircea Trofin, 2016-04-19 05:15:44]

mtrofin@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org, titzer@chromium.org

[Mircea Trofin, 2016-04-19 05:19:40]

Note that Compile shows the improvement in red, because of an
error in reporting (some benchmarks don't have "ms" as units. There's 
a separate CL fixing that).

[titzer, 2016-04-19 08:05:17]

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
src/compiler/instruction.cc:619:
instructions()[instr_index]->set_block_id(block_id);
Can you pull instructions() out of the loop? This always makes me a little
nervous, as I don't trust C++ inlining all that much :-)

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
src/compiler/instruction.h:860: int block_id() const { return block_id_; }
Maybe a DCHECK_NE(kInvalidRpoNumber, block_id_)

[Mircea Trofin, 2016-04-19 15:39:13]

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
src/compiler/instruction.cc:619:
instructions()[instr_index]->set_block_id(block_id);
On 2016/04/19 08:05:17, titzer wrote:
> Can you pull instructions() out of the loop? This always makes me a little
> nervous, as I don't trust C++ inlining all that much :-)

Done.

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/1896813003/diff/20001/src/compiler/instructio...
src/compiler/instruction.h:860: int block_id() const { return block_id_; }
On 2016/04/19 08:05:17, titzer wrote:
> Maybe a DCHECK_NE(kInvalidRpoNumber, block_id_)

Done.

[titzer, 2016-04-19 16:39:50]

https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
src/compiler/instruction.cc:614: void
InstructionSequence::FinishInstructionMap() {
Is it possible to set the instruction id as each instruction is added to the
sequence? I think we have the current block when doing so. That would make this
extra pass unnecessary.

[Mircea Trofin, 2016-04-19 16:56:27]

https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
src/compiler/instruction.cc:614: void
InstructionSequence::FinishInstructionMap() {
On 2016/04/19 16:39:50, titzer wrote:
> Is it possible to set the instruction id as each instruction is added to the
> sequence? I think we have the current block when doing so. That would make
this
> extra pass unnecessary.

I'm not seeing the current block remembered when constructing the sequence. We 
could assume iterative construction, meaning the last block (at the end of the
block list)
is the current block, but I am not very comfortable with that assumption.

Thoughts?

[titzer, 2016-04-20 09:05:48]

On 2016/04/19 16:56:27, Mircea Trofin wrote:
>
https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
> File src/compiler/instruction.cc (right):
> 
>
https://codereview.chromium.org/1896813003/diff/40001/src/compiler/instructio...
> src/compiler/instruction.cc:614: void
> InstructionSequence::FinishInstructionMap() {
> On 2016/04/19 16:39:50, titzer wrote:
> > Is it possible to set the instruction id as each instruction is added to the
> > sequence? I think we have the current block when doing so. That would make
> this
> > extra pass unnecessary.
> 
> I'm not seeing the current block remembered when constructing the sequence. We

> could assume iterative construction, meaning the last block (at the end of the
> block list)
> is the current block, but I am not very comfortable with that assumption.
> 
> Thoughts?

It's OK. Can you just drop in a TODO(turbofan), and maybe we'll get around to it
sometime later.

[titzer, 2016-04-20 09:06:16]

lgtm

[Mircea Trofin, 2016-04-20 20:07:05]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-04-20 20:07:05]

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

[commit-bot: I haz the power, 2016-04-20 20:07:08]

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

[commit-bot: I haz the power, 2016-04-20 20:20:19]

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

[commit-bot: I haz the power, 2016-04-20 20:20:20]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Mircea Trofin, 2016-04-21 01:04:42]

On 2016/04/20 20:20:20, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
>   v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

PTAL - it turns out unit tests weren't going through the same code path where
the blocks would have 
been assigned. I went ahead with the suggestion to assign the blocks when
building the instruction
sequence. In addition, I realized that jump threading may invalidate block
indices, so associating 
instructions with the actual block instead.

[Mircea Trofin, 2016-04-21 22:40:59]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-04-21 22:41:00]

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

[commit-bot: I haz the power, 2016-04-21 22:41:11]

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

[commit-bot: I haz the power, 2016-04-21 23:06:37]

Description was changed from

==========
[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the 
improvement.

BUG=
==========

to

==========
[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the 
improvement.

BUG=
==========

[commit-bot: I haz the power, 2016-04-21 23:06:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:14:58]

Description was changed from

==========
[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the 
improvement.

BUG=
==========

to

==========
[turbofan] store block id with instruction

GetInstructionBlock shows up in some compile time-intensive profiles.
Changing it to a O(1) operation. The compile benchmark confirms the
improvement.

BUG=

Committed: https://crrev.com/81c965d475a51bb9a8df4c4035ccc3ddae1c8980
Cr-Commit-Position: refs/heads/master@{#35711}
==========

[commit-bot: I haz the power, 2016-04-22 19:14:59]

Patchset 6 (id:??) landed as
https://crrev.com/81c965d475a51bb9a8df4c4035ccc3ddae1c8980
Cr-Commit-Position: refs/heads/master@{#35711}