Uniform and precise source positions for inlining

This CL enables precise source positions for all V8 compilers. It merges compiler::SourcePosition and internal::SourcePosition to a single class used throughout the codebase. The new internal::SourcePosition instances store an id identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table DeoptimizationInputData::InliningPositions(), which provides the following data for every inlining id:
 - The inlined SharedFunctionInfo as an offset into DeoptimizationInfo::LiteralArray
 - The SourcePosition of the inlining. Recursively, this yields the full inlining stack.
Before the Code object is created, the same information can be found in CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long as the corresponding Code object is known. The internal represenation of a source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In the case of Turbofan, this required using AstGraphBuilderWithPositions for inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in --trace-deopt and --code-comments. The profiler needs to be updated, at the moment it gets the correct script offsets from the deopt info, but the wrong script id from the reconstructed deopt stack, which can lead to wrong outputs. This should be resolved by making the profiler use the new inlining information for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for Turbofan, changing them to a case where the deopt stack and the inlining position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.) supports now 64bit source positions. Encoding source positions in a single 64bit int together with the difference encoding in the source position table results in very little overhead for the inlining id, since only 12% of the source positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium: LChunk::inlined_functions. This is an analog structure to CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and made Crankshaft use CompilationInfo::inlined_functions instead, because this was necessary to register the offsets into the literal array in a uniform way. This is a safe change because LChunk::inlined_functions has no other uses and the functions in CompilationInfo::inlined_functions have a strictly longer lifespan, being created earlier (in Hydrogen already).

BUG=v8:5432
Committed: https://crrev.com/c3a6ca68d0646b10885ef7017557eaf463db2e4a
Cr-Commit-Position: refs/heads/master@{#40975}

[Tobias Tebbi, 2016-10-25 16:29:55]

Description was changed from

==========
Uniform treatment of source positions for inlining

BUG=v8:5432
==========

to

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kUnknown (-1), it refers to
the outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().

BUG=v8:5432
==========

[Tobias Tebbi, 2016-10-25 16:29:55]

tebbi@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org

[Tobias Tebbi, 2016-10-25 22:37:43]

Description was changed from

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kUnknown (-1), it refers to
the outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().

BUG=v8:5432
==========

to

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kUnknown (-1), it refers to
the outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

[Tobias Tebbi, 2016-10-25 23:25:46]

Patchset #2 (id:20001) has been deleted

[Tobias Tebbi, 2016-10-25 23:35:41]

Patchset #2 (id:30001) has been deleted

[Yang, 2016-10-28 08:15:56]

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

[Yang, 2016-10-28 08:15:57]

Adding vogelheim@ who knows the details of the source position table encoding.

[Michael Starzinger, 2016-10-28 08:45:50]

The compiler-source-position.[h/cc] file seems to be missing. Could you upload
that one.

Also a first few comments that should be applicable throughout the CL.

https://codereview.chromium.org/2451853002/diff/70001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2451853002/diff/70001/src/assembler.cc#newcod...
src/assembler.cc:1898: RecordRelocInfo(RelocInfo::DEOPT_POSITION,
position.ScriptOffset() + 2);
As discussed offline: Could we introduce a constant for this "off-by-two" so
make it visible in the code that the assembler and the deoptimizer are in sync
in that regard?

https://codereview.chromium.org/2451853002/diff/70001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/2451853002/diff/70001/src/assembler.h#newcode46
src/assembler.h:46: #include "src/source-position.h"
nit: I assume this include was added because of the changed signature of
{RecordDeoptReason}. Since it is only used in the signature, we should be able
to get by with a forward declaration in this header file instead.

This applies more broadly throughout the CL: If a forward declaration is
sufficient within a header file, then we usually go with the forward
declaration. That way the include dependency graph for headers stays small,
(re)compilation times decrease and the chance for cyclic dependency is reduced.

https://codereview.chromium.org/2451853002/diff/70001/src/compilation-info.h
File src/compilation-info.h (right):

https://codereview.chromium.org/2451853002/diff/70001/src/compilation-info.h#...
src/compilation-info.h:313: // returns the inlining id for source position
tracking
nit: Please capitalize and punctuate comments everywhere.

[vogelheim, 2016-10-31 17:34:40]

I'm mostly confused about this CL. That might be just me; I'm confused a lot.

I looked mostly at source-position.*, since that seems to be the core of the
change. My comments are somewhat random and probably not super helpful. Maybe we
can discuss this in person? If I have a better understanding of what's going on,
I can probably give more helpful comments.

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h
File src/source-position.h (right):

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:27: SourcePosition() = default;
Wouldn't this leave value_ uninitialized? That doesn't seem like a good idea,
especially in a public method.

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:48: void SetScriptOffset(int script_offset) {
This claims to set the script offset, but for certain values it will actually
silently do nothing. Is this your intent?

Should this maybe have been DCHECKs; or should the SourcePosition have some sort
of 'invalid' state?

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:52: if (script_offset != kUnknown) {
Why not merge the ifs?

if (script_offset != kUnknown &&
    script_offset != kNoSourcePosition &&
    script_offset < ScriptOffsetField::kMax) { ... 


[There's a similar thing below. Note this is largely a matter of taste, so
please decide as you see fit.]

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:66: int EncodeInliningId() { return -(InliningId() + 2); }
why?

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:90: // ids are in the hight bits for better compression in
SourcePositionTable
hight -> high

https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
src/source-position.h:106: // these two ints correspond to the fields in
SourcePosition
"these two ints"? I'm only seeing one int and a SourcePosition.

https://codereview.chromium.org/2451853002/diff/130001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2451853002/diff/130001/test/cctest/cctest.sta...
test/cctest/cctest.status:343: #
'test-cpu-profiler/DeoptAtFirstLevelInlinedSource': [FAIL],
Uh? Did you mean to remove these lines?

[Tobias Tebbi, 2016-11-02 09:50:59]

Patchset #7 (id:150001) has been deleted

[Tobias Tebbi, 2016-11-02 10:11:54]

Thank you so much! Your comments were super helpful!

> https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h
> File src/source-position.h (right):
> 
>
https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
> src/source-position.h:27: SourcePosition() = default;
> Wouldn't this leave value_ uninitialized? That doesn't seem like a good idea,
> especially in a public method.

This was really a bad idea, motivated by C++'s rules for POD. I just removed the
static check in PodArray now, as this seems less dangerous.

>
https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
> src/source-position.h:48: void SetScriptOffset(int script_offset) {
> This claims to set the script offset, but for certain values it will actually
> silently do nothing. Is this your intent?

The intent was some kind of information merging, but this actually conflicts
with how the interface is used in the existing code and conflicts with
expectations. So I removed this now, it should (and is anyway already) better
handled at the client.

> Should this maybe have been DCHECKs; or should the SourcePosition have some
sort
> of 'invalid' state?

Invalid, that is, unknown source positions seem to be unavoidable.
 
>
https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
> src/source-position.h:52: if (script_offset != kUnknown) {
> Why not merge the ifs?
> 
> if (script_offset != kUnknown &&
>     script_offset != kNoSourcePosition &&
>     script_offset < ScriptOffsetField::kMax) { ... 

The first if is to handle overflows (which should not really be necessary
anymore, given the spaceous 31bit encoding of script offsets, but I still left
the code). 
I removed SourcePosition::kUnknown, consistently using the older
kNoSourcePosition and a new, more readable constant SourcePosition::kNotInlined.

> 
>
https://codereview.chromium.org/2451853002/diff/130001/src/source-position.h#...
> src/source-position.h:66: int EncodeInliningId() { return -(InliningId() + 2);
}

This is for encoding source positions to RelocInfo, which stores 32bit ints for
source positions only. The non-overlapping encoding ranges are to avoid
depending on the order in RelocInfo.
For the source position table, I directly encode the 64bit integer, allowing for
better compression.

[Michael Starzinger, 2016-11-03 12:41:44]

Looking good. I like where this is going. One more round of comments, we're
almost there.

But next time please try to make smaller CLs by splitting the work up into
smaller chunks. That reduces the mental overhead for both yourself and your
reviewers.

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.h#newcode56
src/assembler.h:56: class SourcePosition;
nit: Please keep this alpha-sorted.

https://codereview.chromium.org/2451853002/diff/190001/src/compilation-info.cc
File src/compilation-info.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compilation-info.c...
src/compilation-info.cc:226: Handle<PodArray<InliningPosition>>
CompilationInfo::CreateInliningPositions() {
The {CompilationInfo} class should be a pure data container. This method adds
logic. It is specifically not the place to share code between Crankshaft and
TurboFan. Since we already duplicated the {PopulateDeoptimizationData} method, I
would much rather also duplicate this code. Alternatively, if we would like to
share {PopulateDeoptimizationData} or pieces of it then
{v8::internal::CodeGenerator} would be the place, similar to {MakeCodeEpilogue}
there.

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/code-gene...
src/compiler/code-generator.cc:485: masm()->RecordComment(buffer.str().c_str());
Won't the underlying string be freed by the destructor of the {ostringstream}? I
have the feeling that there is a StrDup missing here.

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/compiler-...
File src/compiler/compiler-source-position-table.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.h:5: #ifndef
V8_COMPILER_SOURCE_POSITION_H_
nit: V8_COMPILER_COMPILER_SOURCE_POSITION_TABLE_H_

Also why not just name this file "source-position-table.h" instead? There
shouldn't be any clash with two files having the same name as long as they are
in separate directories. The classes already are in separate namespaces.

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.h:71: #endif  //
V8_COMPILER_SOURCE_POSITION_H_
nit: nit: V8_COMPILER_COMPILER_SOURCE_POSITION_TABLE_H_

https://codereview.chromium.org/2451853002/diff/190001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/factory.h#newcode337
src/factory.h:337: Handle<PodArray<T>> NewPodArray(int length,
How would you call about not adding a new factory method but having this as
{PodArray::New} instead? The object does not add a new instance-type and fully
relies on the underlying ByteArray, no special GC knowledge required. We do this
for other such data structures already. The {Factory} class is considered the
handlification layer of the allocators in the {Heap} class.

https://codereview.chromium.org/2451853002/diff/190001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/objects.cc#newcode...
src/objects.cc:13371: bool Script::GetPositionInfo(int position, PositionInfo*
info,
Note that it inherently unsafe to transition from unhandlified code to
handlified code. Please add a TODO to this method that the entire method and all
call-sites should be handlified.

Especially new call sites in the deoptimizer for tracing are highly problematic.
Because then we potentially trigger crashes in the deoptimizer due to stale
pointer only when tracing is enabled. That would be very unfortunate while
debugging the deoptimizer.

I am borderline OK with landing this as it is, since the problem already existed
before. But I don't think it is wise to ignore this problem for much longer and
would like to follow-up after this CL landed.

https://codereview.chromium.org/2451853002/diff/190001/test/unittests/source-...
File test/unittests/source-position-table-unittest.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/test/unittests/source-...
test/unittests/source-position-table-unittest.cc:26: SourcePosition toPos(int
offset) { return SourcePosition(offset); }
nit: Should either be a member of SourcePositionTableTest or go inside an
anonymous namespace (I personally prefer the former).

[vogelheim, 2016-11-04 10:57:42]

I looked mainly at source-position*. Lots of nitpicks, sorry. :-/

Two things I'm genuinely unhappy about:

- The unit tests in source-position-table-unittest don't seem to test
encoding/decoding the new field at all. That's... not good. Am I overlooking
something?

- I think the encoding in the source-position-table could be better (& simpler
at the same time), if we just encode the ints separately.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
File src/source-position-table.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
src/source-position-table.cc:48: 
[whole file comment, really]

I find the chosen encoding rather weird. When I initially built this, I
admittedly didn't try super hard, but I did try several variants and measured
their impact. [1]

I ended up with two length-encoded ints. You've kept that, but now with one
int32 and one int64, where the int64 is really two separately encoded ints. So
now one 'source int' goes through the length encoding and the
two-in-one-source-position-int as a whole. Does that make any sense?


How about just encoding the three ints separately? Then we can drop the template
magic, and would just call EncodeInt/DecodeInt 3 times. Does that make sense to
you? How does it affect the table size?



[1] (use chromium account:)
https://docs.google.com/spreadsheets/d/1fQLVbTF4eW7RoP1u2AUHG8LMLX65uAhAfqrly...

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
File src/source-position-table.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
src/source-position-table.h:27: PositionTableEntry(int offset, int64_t source,
bool statement)
Why is this (and source_position member) an int64_t and not a SourcePosition?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc
File src/source-position.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc...
src/source-position.cc:66: std::vector<SourcePositionInfo>
SourcePosition::Info(Handle<Code> code) const {
I don't get this. If I read the code below correctly, every code path will
return a vector with a single element in it. Why use a vector, rather than just
returning a SourcePositionInfo?

(Same applies to function above.)

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc...
src/source-position.cc:78: stack.push_back(SourcePositionInfo(*this));
nipitck: (here & function above)

You sometimes build the return value with 
  return std::vector<...>{ value }
and sometimes with
  std::vector<..> result;
  result.push_back( value );
  return result;

Is there any reason for this?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc...
src/source-position.cc:89: out << "<" << ScriptOffset() << ">";
Why not print the inline id?
Also, should there be a representation for the 'magic' values?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h
File src/source-position.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:18: class SharedFunctionInfo;
nitpick: Code < Comp... < Script < SharedFunc...

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:25: class SourcePosition final {
Please add a one-line comment on what this class does.

This class *looks* simple, but I'm rather confused on its value space. So maybe
add a class comment on that, too:

The value space of this class appears to be:
  ( script_offset | kNoSourcePosition ) x ( inline_id | kNotInlined )
  with:
    script_offset = 0..2**31 - 2
    inline_id = 0..2**16 - 2

Can a kNoSourcePosition have a proper inline_id?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:36: bool IsKnown() const { return ScriptOffset() !=
kNoSourcePosition; }
I'm confused.... The CL comment says, "If SourcePosition::InliningId() is
SourcePosition::kUnknown (-1), it refers to the outer (non-inlined) function."
There is no kUnknown. But there is Is[Unk|K]nown(). But those deal with the
script offset, not the inline id.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:36: bool IsKnown() const { return ScriptOffset() !=
kNoSourcePosition; }
Naming (likely connected to my confusion above):

The internal 'magic' values are kNoSourcePosition + kNotInlined. The externally
visible 'magic' values are called known/unknown. That.. doesn't make much sense
to me. If you want special accessors for the 'magic' values - which is probably
a good idea - maybe make them for both. Also, it helps understanding the code if
the naming is consistent.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:49: script_offset = kNoSourcePosition;
What does this if statement guard against? Should it be a DCHECK?

(Generally, we prefer to make bugs explicit, because silently 'fixing' bugs
tends to make vastly more difficult to find & fix them. If this is meant to
protect against a caller putting in non-sensical values, a DCHECK would likely
be better.)

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:54: if (inlining_id >= InliningIdField::kMax) inlining_id
= kNotInlined;
... as above.

Also, if kNotInlined has a specific meaning (... "refers to the outer ..
function" ...), then using it as a 'default' value for invalid input might not
be a good idea.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:60: // The encoding is used for the RelocInfo.
If this is specifically about 32bit integers, why is the type 'int', rather than
int32_t, or something?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:60: // The encoding is used for the RelocInfo.
Why the requirement of "non-overlapping" ints? From a cursory look at the
RelocInfo, it seems like they're distinguishably by position. So why also encode
their 'function' in their sign of the encoded value?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:61: int EncodeScriptOffset() { return ScriptOffset() + 2;
}
Super nitpick, but you're technically constraining the value space further,
since here you forfit the 0 and therefor you can encode one value less.

That's fine IMHO, but if so then your previous checks on the value space should
also check against this limit, not 2**31.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:61: int EncodeScriptOffset() { return ScriptOffset() + 2;
}
const function? (also below)

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:107: int inlined_function_id;
What's the difference/relationship between inlined_function_id and
position.InliningId()?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:111: explicit SourcePositionInfo(SourcePosition position)
: position(position) {}
Ah.... honestly unsure about the C++ subtleties here, but does
position(position) do the right thing=

https://codereview.chromium.org/2451853002/diff/190001/test/unittests/source-...
File test/unittests/source-position-table-unittest.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/test/unittests/source-...
test/unittests/source-position-table-unittest.cc:26: SourcePosition toPos(int
offset) { return SourcePosition(offset); }
I'm rather unhappy about this!

The point of these unit tests is to run the whole encoding/decoding machinery
through its paces. But all the SourcePositions you create here are highly
unusual, because the 2nd parameter is always the default. In other words,
encoding/decoding with inlining ids is 100% untested!!!

So... either extend the unit tests to have meaningful 'inline ids', not merely
default ones. or add new unittests with meaningful 'inline ids'.

[Yang, 2016-11-04 14:32:58]

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

[Yang, 2016-11-04 14:32:59]

+alph, fyi

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.cc#newco...
src/assembler.cc:1899: RecordRelocInfo(RelocInfo::DEOPT_POSITION,
position.EncodeInliningId());
Does it make sense to have one single reloc info entry for both? Or is changing
the reloc info format for this not worth saving the reloc info header?

Also, would it make sense to store this position information into the separate
source position table instead?

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/compiler-...
File src/compiler/compiler-source-position-table.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.cc:64: << ":" << pos.ScriptOffset();
Since this is used for debugging anyways, can we include every piece of
information we can extract?

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/graph-vis...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/graph-vis...
src/compiler/graph-visualizer.cc:155: os_ << ",\"pos\":" <<
position.ScriptOffset();
Also here, include inlining id as well?

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/graph-vis...
src/compiler/graph-visualizer.cc:500: os_ << " pos:" << position.ScriptOffset();
Same here.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
File src/source-position-table.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
src/source-position-table.cc:48: 
On 2016/11/04 10:57:41, vogelheim wrote:
> [whole file comment, really]
> 
> I find the chosen encoding rather weird. When I initially built this, I
> admittedly didn't try super hard, but I did try several variants and measured
> their impact. [1]
> 
> I ended up with two length-encoded ints. You've kept that, but now with one
> int32 and one int64, where the int64 is really two separately encoded ints. So
> now one 'source int' goes through the length encoding and the
> two-in-one-source-position-int as a whole. Does that make any sense?
> 
> 
> How about just encoding the three ints separately? Then we can drop the
template
> magic, and would just call EncodeInt/DecodeInt 3 times. Does that make sense
to
> you? How does it affect the table size?
> 
> 
> 
> [1] (use chromium account:)
>
https://docs.google.com/spreadsheets/d/1fQLVbTF4eW7RoP1u2AUHG8LMLX65uAhAfqrly...

Well since this is delta encoded, simply calling EncodeInt 3 times would
certainly be inefficient. The delta would have to refer to the EncodeInt 3 calls
ago.

Which would probably be a lot easier to reason about and also be more efficient.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc
File src/source-position.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.cc...
src/source-position.cc:66: std::vector<SourcePositionInfo>
SourcePosition::Info(Handle<Code> code) const {
On 2016/11/04 10:57:41, vogelheim wrote:
> I don't get this. If I read the code below correctly, every code path will
> return a vector with a single element in it. Why use a vector, rather than
just
> returning a SourcePositionInfo?
> 
> (Same applies to function above.)

I guess the vector is supposed to return the full inlining stack. This method
should be named accordingly.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h
File src/source-position.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:60: // The encoding is used for the RelocInfo.
On 2016/11/04 10:57:42, vogelheim wrote:
> If this is specifically about 32bit integers, why is the type 'int', rather
than
> int32_t, or something?

I would also suggest using uint32_t

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:107: int inlined_function_id;
On 2016/11/04 10:57:42, vogelheim wrote:
> What's the difference/relationship between inlined_function_id and
> position.InliningId()?

I'm also surprised that this needs to be an additional information. Isn't the
point of the inlining id in SourcePosition to be an index into the literals
array?

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:111: explicit SourcePositionInfo(SourcePosition position)
: position(position) {}
On 2016/11/04 10:57:41, vogelheim wrote:
> Ah.... honestly unsure about the C++ subtleties here, but does
> position(position) do the right thing=

Yeah. Let's rename the parameter here for readability.

[Tobias Tebbi, 2016-11-07 14:00:58]

Description was changed from

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kUnknown (-1), it refers to
the outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

to

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the
outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions. Encoding source positions in a single 64bit
int together with the difference encoding in the source position table results
in very little overhead for the inlining id, since only 12% of the source
positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

[Tobias Tebbi, 2016-11-07 15:10:34]

I addressed the comments (especialy refactored Script::GetPositionInfo to
properly separate handlified and unhandlified code, and simplified the encoding
in RelocInfo). Due to rebasing, the diffs are pretty unreadable now. I don't
know how to fix this.

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/assembler.cc#newco...
src/assembler.cc:1899: RecordRelocInfo(RelocInfo::DEOPT_POSITION,
position.EncodeInliningId());
On 2016/11/04 14:32:59, Yang wrote:
> Does it make sense to have one single reloc info entry for both? Or is
changing
> the reloc info format for this not worth saving the reloc info header?
> 
> Also, would it make sense to store this position information into the separate
> source position table instead?

Changing the RelocInfo entry seems difficult, because RelocInfo only has an
intptr_t data field. Puting the source position in the SourcePositionTable seems
reasonable, but I consider this too big a change to meld into this already
oversized CL.

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/graph-vis...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/compiler/graph-vis...
src/compiler/graph-visualizer.cc:155: os_ << ",\"pos\":" <<
position.ScriptOffset();
On 2016/11/04 14:32:59, Yang wrote:
> Also here, include inlining id as well?

Yes, but Turbolizer also needs support for this, so I would also like to do this
in a separate CL.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
File src/source-position-table.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
src/source-position-table.cc:48: 
On 2016/11/04 14:32:59, Yang wrote:
> On 2016/11/04 10:57:41, vogelheim wrote:
> > [whole file comment, really]
> > 
> > I find the chosen encoding rather weird. When I initially built this, I
> > admittedly didn't try super hard, but I did try several variants and
measured
> > their impact. [1]
> > 
> > I ended up with two length-encoded ints. You've kept that, but now with one
> > int32 and one int64, where the int64 is really two separately encoded ints.
So
> > now one 'source int' goes through the length encoding and the
> > two-in-one-source-position-int as a whole. Does that make any sense?
> > 
> > 
> > How about just encoding the three ints separately? Then we can drop the
> template
> > magic, and would just call EncodeInt/DecodeInt 3 times. Does that make sense
> to
> > you? How does it affect the table size?
> > 
> > 
> > 
> > [1] (use chromium account:)
> >
>
https://docs.google.com/spreadsheets/d/1fQLVbTF4eW7RoP1u2AUHG8LMLX65uAhAfqrly...
> 
> Well since this is delta encoded, simply calling EncodeInt 3 times would
> certainly be inefficient. The delta would have to refer to the EncodeInt 3
calls
> ago.
> 
> Which would probably be a lot easier to reason about and also be more
efficient.

I tried the version with 3 encoded ints now and it leads to a 39% overhead
compared to the earlier solution with a single 64bit int. This is due to the
fact that 88% of the source positions keep the previous inlining_id.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h
File src/source-position.h (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:25: class SourcePosition final {
On 2016/11/04 10:57:41, vogelheim wrote:
> Can a kNoSourcePosition have a proper inline_id?

It has a proper interpretation (unknown position inside of an inlined call), and
it is currently used.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:60: // The encoding is used for the RelocInfo.
On 2016/11/04 10:57:42, vogelheim wrote:
> Why the requirement of "non-overlapping" ints? From a cursory look at the
> RelocInfo, it seems like they're distinguishably by position. So why also
encode
> their 'function' in their sign of the encoded value?

I don't have a very good reason to do this indeed, I switched to a positional
encoding now.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position.h#...
src/source-position.h:107: int inlined_function_id;
On 2016/11/04 14:32:59, Yang wrote:
> On 2016/11/04 10:57:42, vogelheim wrote:
> > What's the difference/relationship between inlined_function_id and
> > position.InliningId()?
> 
> I'm also surprised that this needs to be an additional information. Isn't the
> point of the inlining id in SourcePosition to be an index into the literals
> array?

DeoptimizationInputData::literals() can have different positions than  the
inlining ids, because the existing code removes self-inlinings from it. To avoid
changing this part I don't know the motivations for and to avoid adding another
brittle (and already violated!) global invariant, I kept these separate.

[vogelheim, 2016-11-07 17:53:27]

lgtm (from my perspective; but please wait for other reviewers' feedback, as I
don't know most of the code you've touched very well)

Some more nitpicks, mostly style stuff. Please do have a look, particularly at
the code duplication between code-generator.cc and lithium-codegen.cc.

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
File src/source-position-table.cc (right):

https://codereview.chromium.org/2451853002/diff/190001/src/source-position-ta...
src/source-position-table.cc:48: 
On 2016/11/07 15:10:34, Tobias Tebbi wrote:
> On 2016/11/04 14:32:59, Yang wrote:
> > On 2016/11/04 10:57:41, vogelheim wrote:
> > > [whole file comment, really]
> > > 
> > > I find the chosen encoding rather weird. When I initially built this, I
> > > admittedly didn't try super hard, but I did try several variants and
> measured
> > > their impact. [1]
> > > 
> > > I ended up with two length-encoded ints. You've kept that, but now with
one
> > > int32 and one int64, where the int64 is really two separately encoded
ints.
> So
> > > now one 'source int' goes through the length encoding and the
> > > two-in-one-source-position-int as a whole. Does that make any sense?
> > > 
> > > 
> > > How about just encoding the three ints separately? Then we can drop the
> > template
> > > magic, and would just call EncodeInt/DecodeInt 3 times. Does that make
sense
> > to
> > > you? How does it affect the table size?
> > > 
> > > 
> > > 
> > > [1] (use chromium account:)
> > >
> >
>
https://docs.google.com/spreadsheets/d/1fQLVbTF4eW7RoP1u2AUHG8LMLX65uAhAfqrly...
> > 
> > Well since this is delta encoded, simply calling EncodeInt 3 times would
> > certainly be inefficient. The delta would have to refer to the EncodeInt 3
> calls
> > ago.
> > 
> > Which would probably be a lot easier to reason about and also be more
> efficient.
> 
> I tried the version with 3 encoded ints now and it leads to a 39% overhead
> compared to the earlier solution with a single 64bit int. This is due to the
> fact that 88% of the source positions keep the previous inlining_id.

Thank you! Sounds like your version is indeed better. :-)

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:22: // Forward declarations.
nitpick: Drop comment. It's indeed a forward declaration, but the reader will
realize this without the comment.

Reference:
https://google.github.io/styleguide/cppguide.html#Implementation_Comments,
subsection "Don'ts": "Do not state the obvious. In particular, don't literally
describe what code does [...]"

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/code-gene...
src/compiler/code-generator.cc:516: if (inlined_functions.size() == 0)
style nitpick: use if with braces, if it extends to more than one line.

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/compiler-...
File src/compiler/compiler-source-position-table.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.h:52: void
SetCurrentPosition(SourcePosition pos) { current_position_ = pos; }
nitpick: const SourcePosition& ?? I think this will copy it twice.

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/instructi...
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/instructi...
src/compiler/instruction.h:26: // Forward declarations.
nitpick: Drop comment. (As explained elsewhere. Please also drop the forward
decl comment below, even though you didn't add it.)

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/lithium...
File src/crankshaft/lithium-codegen.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/lithium...
src/crankshaft/lithium-codegen.cc:315: CompilationInfo* info) {
Hrmm... This looks like an exact copy of CreateInliningPositions in
code-generator.cc. Is there a better place to put this, where both call sites
could reference it? If not, why not?

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/lithium...
src/crankshaft/lithium-codegen.cc:318: if (inlined_functions.size() == 0)
nitpick: use braces if more than one line.

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/mips/li...
File src/crankshaft/mips/lithium-mips.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/mips/li...
src/crankshaft/mips/lithium-mips.cc:2294: 
[Here, and in pretty much all src/crankshaft/*/lithium-*.cc:]

Why the empty line?

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h#newcode7214
src/objects.h:7214: static int GetColumnNumber(Handle<Script> script, int
code_offset);
The static instances get GetPositionInfo, GetColumnNumber and GetLineNumber look
like some rather gratuitous code duplication. Is there a reason why we can't
just call script->GetXXXX instead? If something is missing, can we make the
non-static members do that additional work instead?

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h#newcode7215
src/objects.h:7215: int GetColumnNumber(int code_pos);
const method?

[Also GetLineNumber below. And I get GetPositionInfo above..

It looks like making GetPositionInfo const would require ends() to be const,
which requires the underlying member to be mutable. Not sure if any of this is
worth the effort, though. You decide.]

https://codereview.chromium.org/2451853002/diff/210001/src/source-position.h
File src/source-position.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/source-position.h#...
src/source-position.h:33: class SourcePosition final {
How about:

SourcePosition stores
- script_offset (31 bit non-negative int or kNoSourcePosition)
- inlining_id (16 bit non-negative int or kNotInlined).

A defined inlining_id refers to positions in CompilationInfo::inlined_functions
or DeoptimizationInputData::InliningPositions, depending on the compilation
stage.

https://codereview.chromium.org/2451853002/diff/210001/test/unittests/source-...
File test/unittests/source-position-table-unittest.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/test/unittests/source-...
test/unittests/source-position-table-unittest.cc:20: SourcePosition toPos(int
offset) {
const

[vogelheim, 2016-11-07 18:10:39]

On 2016/11/07 15:10:34, Tobias Tebbi wrote:
> I addressed the comments (especialy refactored Script::GetPositionInfo to
> properly separate handlified and unhandlified code, and simplified the
encoding
> in RelocInfo). Due to rebasing, the diffs are pretty unreadable now. I don't
> know how to fix this.

The best way to handle this is to upload one CL with only the rebase - but no
'real' changes - and then upload the changes separately. 

That unfortunately requires one to realize the issue beforehand, and I mostly
forget. In the few cases where it was really, really messy I fixed this after
the fact by:
0, [inadvertently uploaded a CL which mixes rebase + actual changes]
1, reverting my client to the old state
2, then doing another rebase
3, then upload the CL
4, then patch in the previously-uploaded CL into the client
5, the upload that CL
6, then go into the review tool and delete the original CL from #0.

That is, admittedly, a lot of work, and I only bother if I think there's a
significant problem.

[alph, 2016-11-07 22:38:40]

Great! Thanks for doing this.

https://codereview.chromium.org/2451853002/diff/210001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/assembler.cc#newco...
src/assembler.cc:1899: RecordRelocInfo(RelocInfo::DEOPT_POSITION,
position.InliningId());
Hmm, looks hacky. How about adding a new tag for inlining id?

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.h:628: int inlining_id = -1);
nit: = SourcePosition::kNotInlined

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:325: int const inlining_id_;
Isn't inlining_id already a part of start_position_ ?

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:1368: TraceInlinedFunction(info_->shared_info(),
SourcePosition::Unknown(), -1);
Is there a name for -1?

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc#newcode...
src/objects.cc:13433: if (!GetPositionInfoSlow(this, position, info)) return
false;
Why the change? It's gonna be slow on repetitive calls with no line_ends.

[Tobias Tebbi, 2016-11-08 10:29:07]

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/lithium...
File src/crankshaft/lithium-codegen.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/crankshaft/lithium...
src/crankshaft/lithium-codegen.cc:315: CompilationInfo* info) {
On 2016/11/07 17:53:27, vogelheim wrote:
> Hrmm... This looks like an exact copy of CreateInliningPositions in
> code-generator.cc. Is there a better place to put this, where both call sites
> could reference it? If not, why not?

mstarzinger@ suggested this duplication, because we already duplicated
PopulateDeoptimizationData.

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc#newcode...
src/objects.cc:13433: if (!GetPositionInfoSlow(this, position, info)) return
false;
On 2016/11/07 22:38:39, alph wrote:
> Why the change? It's gonna be slow on repetitive calls with no line_ends.

Yes, because this is the safe unhandlified version. Handlified code should call
the static, handlified variant. I changed all the callsites.

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h#newcode7214
src/objects.h:7214: static int GetColumnNumber(Handle<Script> script, int
code_offset);
On 2016/11/07 17:53:27, vogelheim wrote:
> The static instances get GetPositionInfo, GetColumnNumber and GetLineNumber
look
> like some rather gratuitous code duplication. Is there a reason why we can't
> just call script->GetXXXX instead? If something is missing, can we make the
> non-static members do that additional work instead?

The reason is that the non-static methods are not allocating, but may be slower
because the cannot initialize the line ends array. So handlified callsites
should use the handlified version for performace, while unhandlified callsites
have to resort to the non-static methods. I will clearify the above comment.

https://codereview.chromium.org/2451853002/diff/210001/src/objects.h#newcode7215
src/objects.h:7215: int GetColumnNumber(int code_pos);
On 2016/11/07 17:53:27, vogelheim wrote:
> const method?
> 
> [Also GetLineNumber below. And I get GetPositionInfo above..
> 
> It looks like making GetPositionInfo const would require ends() to be const,
> which requires the underlying member to be mutable. Not sure if any of this is
> worth the effort, though. You decide.]

Since the underlying member is not even a C++ member, this would require const
casting. Also, the C++11 lib spec requires const methods to be thread-safe
(http://stackoverflow.com/questions/14127379/does-const-mean-thread-safe-in-c11),
so mutable should only be used on locked objects. On the other hand, this update
of a cache should even be a case of lock-free thread safety... And I am not sure
if this applies to V8 at all.

[Yang, 2016-11-08 13:32:13]

lgtm for everything aside from compilers.

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc#newcode...
src/objects.cc:13433: if (!GetPositionInfoSlow(this, position, info)) return
false;
On 2016/11/07 22:38:39, alph wrote:
> Why the change? It's gonna be slow on repetitive calls with no line_ends.

Yes, but assuming that we migrate to the handlified version of GetPositionInfo
where possible, we would initialize line_ends array upfront. Only for cases
where we can't use the handlified version we would go the slow path. But in
these cases, we must not allocate to begin with.

https://codereview.chromium.org/2451853002/diff/210001/src/objects.cc#newcode...
src/objects.cc:13502: Script::GetPositionInfo(script, code_pos, &info,
WITH_OFFSET);
The "Script::" is redundant here. Same below.

https://codereview.chromium.org/2451853002/diff/250001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2451853002/diff/250001/src/factory.cc#newcode...
src/factory.cc:1035: if (FLAG_trace_deopt) Script::InitLineEnds(script);
Can we do this at the end so that we have a fully initialized script object?

Also, this probably doesn't make any sense for wasm scripts, or our own js
builtins. So I would prefer doing this at the call sites of NewScript. Of
course, for FLAG_trace_deopt this simple solution probably suffices.

[Michael Starzinger, 2016-11-11 11:39:30]

LGTM.

https://codereview.chromium.org/2451853002/diff/290001/src/compiler/compiler-...
File src/compiler/compiler-source-position-table.h (right):

https://codereview.chromium.org/2451853002/diff/290001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.h:5: #ifndef
V8_COMPILER_SOURCE_POSITION_TABLE_H_
nit: V8_COMPILER_COMPILER_SOURCE_POSITION_TABLE_H_

https://codereview.chromium.org/2451853002/diff/290001/src/compiler/compiler-...
src/compiler/compiler-source-position-table.h:73: #endif  //
V8_COMPILER_SOURCE_POSITION_TABLE_H_
nit: V8_COMPILER_COMPILER_SOURCE_POSITION_TABLE_H_

[Tobias Tebbi, 2016-11-11 11:45:31]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-11 11:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org, yangguo@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2451853002/#ps310001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-11 11:45:40]

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-11-11 11:47:08]

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

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

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/16057)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16100)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16018)

[Tobias Tebbi, 2016-11-11 11:52:52]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-11 11:52:53]

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

[commit-bot: I haz the power, 2016-11-11 11:53:01]

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-11-11 11:57:10]

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

[commit-bot: I haz the power, 2016-11-11 11:57:12]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[Benedikt Meurer, 2016-11-11 11:58:17]

lgtm

[Tobias Tebbi, 2016-11-11 12:11:52]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-11 12:11:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2451853002/#ps350001
(title: "fixed rebase issues")

[commit-bot: I haz the power, 2016-11-11 12:11:58]

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-11-11 12:18:06]

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

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

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

[Tobias Tebbi, 2016-11-11 12:19:10]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-11 12:19:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2451853002/#ps370001
(title: "fixed compile issue")

[commit-bot: I haz the power, 2016-11-11 12:19:18]

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-11-11 12:26:41]

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

[commit-bot: I haz the power, 2016-11-11 12:26:42]

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

[Tobias Tebbi, 2016-11-14 10:29:03]

Patchset #18 (id:390001) has been deleted

[Tobias Tebbi, 2016-11-14 10:49:26]

Patchset #18 (id:410001) has been deleted

[Tobias Tebbi, 2016-11-14 15:23:52]

Patchset #25 (id:560001) has been deleted

[Tobias Tebbi, 2016-11-14 15:23:56]

Patchset #24 (id:540001) has been deleted

[Tobias Tebbi, 2016-11-14 15:24:04]

Patchset #23 (id:520001) has been deleted

[Tobias Tebbi, 2016-11-14 15:24:15]

Patchset #22 (id:500001) has been deleted

[Tobias Tebbi, 2016-11-14 15:24:29]

Patchset #21 (id:480001) has been deleted

[Tobias Tebbi, 2016-11-14 15:24:53]

Patchset #20 (id:470001) has been deleted

[Tobias Tebbi, 2016-11-14 15:25:06]

Patchset #19 (id:450001) has been deleted

[Tobias Tebbi, 2016-11-14 15:25:18]

Patchset #18 (id:430001) has been deleted

[Tobias Tebbi, 2016-11-14 15:26:48]

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

[commit-bot: I haz the power, 2016-11-14 15:26:59]

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-11-14 15:30:08]

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

[commit-bot: I haz the power, 2016-11-14 15:30:10]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[Tobias Tebbi, 2016-11-14 15:33:47]

Patchset #18 (id:580001) has been deleted

[Tobias Tebbi, 2016-11-14 15:43:27]

tebbi@chromium.org changed reviewers:
- alph@chromium.org, bmeurer@chromium.org, vogelheim@chromium.org,
yangguo@chromium.org

[Tobias Tebbi, 2016-11-14 15:43:29]

Hi Michi,

Could you have a fast look at the changes in src/compiler/node-aux-data.h? These
were necessary because MSVC insisted on a trivial constructor for SourcePosition
since SourcePosition is used in a union by the profiler.
So I had to make the default constructor trivial and hide it as private.
Consequently, I had to make node-aux-data.h no longer use the default
constructor.

Thank you!

Best,
Tobias

[Tobias Tebbi, 2016-11-14 15:44:08]

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

[commit-bot: I haz the power, 2016-11-14 15:44:20]

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-11-14 15:48:08]

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

[commit-bot: I haz the power, 2016-11-14 15:48:10]

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

[Michael Starzinger, 2016-11-14 15:53:40]

LGTM on node-aux-data changes.

https://codereview.chromium.org/2451853002/diff/600001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2451853002/diff/600001/src/compiler/code-gene...
src/compiler/code-generator.cc:523: for (unsigned i = 0; i <
inlined_functions.size(); ++i) {
nit: s/unsigned/size_t/ here.

[Tobias Tebbi, 2016-11-14 16:10:17]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-14 16:10:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org, vogelheim@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2451853002/#ps640001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-14 16:10:30]

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-11-14 16:47:16]

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

[commit-bot: I haz the power, 2016-11-14 16:47:22]

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

[Tobias Tebbi, 2016-11-14 16:53:29]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2016-11-14 16:53:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, vogelheim@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2451853002/#ps660001
(title: "fixed gcmole issue")

[commit-bot: I haz the power, 2016-11-14 16:53:40]

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-11-14 17:21:48]

Description was changed from

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the
outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions. Encoding source positions in a single 64bit
int together with the difference encoding in the source position table results
in very little overhead for the inlining id, since only 12% of the source
positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

to

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the
outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions. Encoding source positions in a single 64bit
int together with the difference encoding in the source position table results
in very little overhead for the inlining id, since only 12% of the source
positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

[commit-bot: I haz the power, 2016-11-14 17:21:50]

Committed patchset #21 (id:660001)

[commit-bot: I haz the power, 2016-11-17 22:33:08]

Description was changed from

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray 
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the
outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions. Encoding source positions in a single 64bit
int together with the difference encoding in the source position table results
in very little overhead for the inlining id, since only 12% of the source
positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432
==========

to

==========
This CL enables precise source positions for all V8 compilers. It merges
compiler::SourcePosition and internal::SourcePosition to a single class used
throughout the codebase. The new internal::SourcePosition instances store an id
identifying an inlined function in addition to a script offset.
SourcePosition::InliningId() refers to a the new table
DeoptimizationInputData::InliningPositions(), which provides the following data
for every inlining id:
 - The inlined SharedFunctionInfo as an offset into
DeoptimizationInfo::LiteralArray
 - The SourcePosition of the inlining. Recursively, this yields the full
inlining stack.
Before the Code object is created, the same information can be found in
CompilationInfo::inlined_functions().

If SourcePosition::InliningId() is SourcePosition::kNotInlined, it refers to the
outer (non-inlined) function.
So every SourcePosition has full information about its inlining stack, as long
as the corresponding Code object is known. The internal represenation of a
source position is a positive 64bit integer.

All compilers create now appropriate source positions for inlined functions. In
the case of Turbofan, this required using AstGraphBuilderWithPositions for
inlined functions too. So this class is now moved to a header file.

At the moment, the additional information in source positions is only used in
--trace-deopt and --code-comments. The profiler needs to be updated, at the
moment it gets the correct script offsets from the deopt info, but the wrong
script id from the reconstructed deopt stack, which can lead to wrong outputs.
This should be resolved by making the profiler use the new inlining information
for deopts.

I activated the inlined deoptimization tests in test-cpu-profiler.cc for
Turbofan, changing them to a case where the deopt stack and the inlining
position agree. It is currently still broken for other cases.

The following additional changes were necessary:
 - The source position table (internal::SourcePositionTableBuilder etc.)
supports now 64bit source positions. Encoding source positions in a single 64bit
int together with the difference encoding in the source position table results
in very little overhead for the inlining id, since only 12% of the source
positions in Octane have a changed inlining id.
 - The class HPositionInfo was effectively dead code and is now removed.
 - SourcePosition has new printing and information facilities, including
computing a full inlining stack.
 - I had to rename compiler/source-position.{h,cc} to
compiler/compiler-source-position-table.{h,cc} to avoid clashes with the new
src/source-position.cc file.
 - I wrote the new wrapper PodArray for ByteArray. It is a template working with
any POD-type. This is used in DeoptimizationInputData::InliningPositions().
 - I removed HInlinedFunctionInfo and HGraph::inlined_function_infos, because
they were only used for the now obsolete Crankshaft inlining ids.
 - Crankshaft managed a list of inlined functions in Lithium:
LChunk::inlined_functions. This is an analog structure to
CompilationInfo::inlined_functions. So I removed LChunk::inlined_functions and
made Crankshaft use CompilationInfo::inlined_functions instead, because this was
necessary to register the offsets into the literal array in a uniform way. This
is a safe change because LChunk::inlined_functions has no other uses and the
functions in CompilationInfo::inlined_functions have a strictly longer lifespan,
being created earlier (in Hydrogen already).

BUG=v8:5432

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

[commit-bot: I haz the power, 2016-11-17 22:33:10]

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

[Vyacheslav Egorov (Google), 2016-12-12 21:19:45]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2016-12-12 21:19:47]

https://codereview.chromium.org/2451853002/diff/660001/src/crankshaft/hydroge...
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2451853002/diff/660001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:12811: if (pos.isInlined()) os << "inlining(" <<
pos.InliningId() << "),";
This change has broken IRHydra (https://github.com/mraleph/irhydra/issues/53)

I would have appreciated some heads up.

https://codereview.chromium.org/2451853002/diff/660001/src/crankshaft/hydroge...
src/crankshaft/hydrogen.cc:12812: os << pos.ScriptOffset();
Notice that previously this was offset since the beginning of function literal
for two reasons:

1. save some bits (this reason is obsolete with your change)
2. actually make this offset useful - we dump only function source, not the
whole script (HGraphBuilder::TraceInlinedFunction) so offset from the beginning
of the script needs to be converted to the offset since the start of the
function if we want to show something (e.g. display where deoptimization
happened based on that dumped code). Tool like IRHydra has only access to what
is dumped.