Patch to enhance the source code line information for profiler.

Patch to enhance the source code line information for profiler.

This patch is to enhance the source code line information for profiler.

For the Hydrogen compilation, most of the source code line information
is not copied from the HInstruction the to corresponding LInstruction.

This patch defines one PositionBits field for LInstruction and copies the
sorce code position value from the HInstruction.  

When Generating the native code, we use RecordPosition(..) function to
write LInstruction's position value to position recorder.

For the MIPS platform, I did not touch because I have no devices
to verify the modification on it.

[danno, 2013-07-29 12:29:01]

My comments below apply to all platforms. Please also do the port to ARM, which
by default runs with an simulator on ia32/x64 so you  can run tests without ARM
hardware:

make arm.check

A MIPS is optional, since the MIPS folks can port your change as they do other
changes to the core V8 platforms.

https://codereview.chromium.org/21042003/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/21042003/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:354: if (pos >= 0) RecordPosition(pos);
In order to optimize the size of the source pos tables, don't output the pos
again if it's the same as the pos for the last instruction.

https://codereview.chromium.org/21042003/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:356: instr->CompileToNative(this);
Not that your solution doesn't handle deferred code correctly, which is all
output at the end of a routine. Please take a look at GenerateDeferredCode.

https://codereview.chromium.org/21042003/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/21042003/diff/1/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:306: int position_;
Please turn this into a bit field with is_call using the BitField class so that
it doesn't use an extra word in the LInstruction class.

[chunyang.dai, 2013-07-30 10:37:54]

Hello, Daniel.
For the comments in lithim-ia32.h:

If I use BitField to represent both is_call and position value, which type
should be used to define the whole data? because the "position"
is int. And if I define one int to represent this two value, it will report
error. If we define it as "long", is it too big?

[danno, 2013-07-30 10:55:20]

I suggest using bit fields as we do elsewhere in Hydrogen, perhaps defining:

  class IsCallField:       public BitField<bool, 0, 1> {};
  class PositionField:   public BitField<int,  1, 31> {};
 
  int bit_field_;

so the actual storage of the bit field is only 32 bits.

[chunyang.dai, 2013-07-30 14:50:05]

Hello, Daniel.
I implemented the same code according to your suggestion. But the position value
may be 
the RelocInfo::kNoPosition. It's -1.  And when we invoke
PositionField::encode(-1)
or PositionField::update(fit_field_, -1), the assertion in encode function will
fail.

[chunyang.dai, 2013-07-30 14:50:08]

Hello, Daniel.
I implemented the same code according to your suggestion. But the position value
may be 
the RelocInfo::kNoPosition. It's -1.  And when we invoke
PositionField::encode(-1)
or PositionField::update(fit_field_, -1), the assertion in encode function will
fail.

[danno, 2013-07-31 08:29:20]

Can you not store (position + 1) in the encoded field? If you protect access
with accessors that always +1/-1, then the callers of the accessor function will
not notice.

[chunyang.dai, 2013-08-01 07:48:24]

Hello, Daniel.
I updated the code and please review it.
Thanks a lot for this smart advice.

[chunyang.dai, 2013-08-02 10:14:21]

Hello. Daniel.
Could you please review this patch?
Thanks.

[danno, 2013-08-02 10:53:16]

Note that all comments for ARM apply to other platforms, too.

Please note that I have a pretty high review load, so I can't always guarantee a
24-hour review turn around. I think you should work under the assumption that
reviews might take several days, although I will try to keep the turn around
time for reviews as low as possible.

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-arm.h#new...
src/arm/lithium-arm.h:216: }
Build the constructor like this instead:

LInstruction() : environment_(NULL), hydrogen_value_(NULL) 
    : bit_field_(IsCallBits::encode(false)) {
  set_position(RelocInfo::kNoPosition);
}

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-arm.h#new...
src/arm/lithium-arm.h:273: bool ClobbersTemps() const { return
IsCallBits::decode(bit_field_); }
There should only be a single function that explicitly uses
"IsCallBits::decode(bit_field_)", and that should be a public function "bool
IsCall() const {  return IsCallBits::decode(bit_field_); }" The other usages in
this class should call the wrapper function IsCall rather than decoding the bits
directly.

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-arm.h#new...
src/arm/lithium-arm.h:303: 
coding standard nit: Please put the class definitions of the BitFields here, not
below.

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/21042003/diff/14001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:299: if (pos >=0) RecordPosition(pos);
Since code is ouput linearly, I think it makes sense to have a old_position
member on the codegen object and a method RecordAndUpdatePosition(pos) that has
the logic that encapsulates the logic for eliding repeating positions, and call
that thing here and above. Same applies to other platforms.

[chunyang.dai, 2013-08-05 16:05:48]

Hello, Daniel.
I am sorry I send too many messages to notify you. 
This time I updated the code according to your comments. Please review it.

[danno, 2013-08-05 16:59:16]

Please make sure that your patch works for all platforms by running:

make check

https://codereview.chromium.org/21042003/diff/28001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/21042003/diff/28001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:259: int old_position = RelocInfo::kNoPosition;
old_position is now unused, correct? Please remove.

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:338: 
nit: please remove extraneous whitespace change.

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:1178: void
LCodeGen::RecordAndUpdatePosition(int position) {
Don't you need this in the ARM implementation, too? Does it actually compile/run
on ARM?

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:271: 
Remove the line between MarkAsCall and IsCall

https://codereview.chromium.org/21042003/diff/28001/src/ia32/lithium-ia32.h#n...
src/ia32/lithium-ia32.h:272: bool IsCall() const { return
IsCallBits::decode(bit_field_); }
Add a new line after IsCall

https://codereview.chromium.org/21042003/diff/28001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/21042003/diff/28001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:263: 
Please remove extraneous whitespace change

[chunyang.dai, 2013-08-06 10:08:22]

hello. Daniel.
I am sorry I uploaded the incorrect code for code review because I just want to
upload the code before I leave office. So I did not check it carefully and run 
"make arm.check" according to your suggestion. 

Now I updated the code and checked it carefully. Thanks a lot for your review
comments.

[danno, 2013-08-07 19:10:26]

lgtm

[haitao.feng, 2013-08-08 02:27:46]

I have helped to commit this CL as r16114.