Convert UnaryOpStub to a HydrogenCodeStub

Convert UnaryOpStub to a HydrogenCodeStub

BUG=
R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15473

[danno, 2013-06-21 16:38:04]

I _really_ like the direction this is going.

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

https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:2164: int left_block =
instr->TrueDestination(chunk_);
Why this unrelated change?

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:769:
minus->AssumeRepresentation(Representation::Double());
Try to move this out with the other representation changes so that this code can
be shared 100% with hydrogen.

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:781: UnaryOpStub* stub = casted_stub();
Please share this code with hydrogen, it should be identical to the inlined
instructions in the optimized code. We do this for other stubs by putting

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:802: AddInstruction(new(zone()) HCheckSmi(input));
Try to avoid the explicit checks by using force representation here and below
for double.

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs.cc#newcode214
src/code-stubs.cc:214: 
nit: two empty lines, please

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs.cc#newcode227
src/code-stubs.cc:227: void UnaryOpStub::Record(Handle<Object> object) {
UpdateState?

https://codereview.chromium.org/17229005/diff/23028/src/code-stubs.cc#newcode242
src/code-stubs.cc:242: 
nit: two spaces between functions

https://codereview.chromium.org/17229005/diff/23028/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/17229005/diff/23028/src/ic.cc#newcode2479
src/ic.cc:2479: 
nit: two lines between functions

https://codereview.chromium.org/17229005/diff/23028/src/ic.cc#newcode2484
src/ic.cc:2484: stub.Record(object);
Can you change "Record" here and elsewhere to UpdateState()?

[Rodolph Perfetta, 2013-06-21 18:32:38]

drive by comments on ARM.

https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2036: LOperand* value = UseAtStart(instr->value());
Shouldn't this be UseRegisterAtStart?

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

https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:2199: __ ldr(ip, FieldMemOperand(reg,
HeapObject::kMapOffset));
ip shouldn't be used outside the macro assembler. CompareRoot below will use ip
as a temp register to do the compare so in the end you will always branch (or
assert in debug).

Lithium has a dedicated scratch register: scratch0() which you can use instead.

[oliv, 2013-06-24 09:05:44]

On 2013/06/21 18:32:38, Rodolph Perfetta wrote:
> drive by comments on ARM.
> 
> https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-arm.cc
> File src/arm/lithium-arm.cc (right):
> 
>
https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-arm.cc#ne...
> src/arm/lithium-arm.cc:2036: LOperand* value = UseAtStart(instr->value());
> Shouldn't this be UseRegisterAtStart?
> 
>
https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-codegen-a...
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-codegen-a...
> src/arm/lithium-codegen-arm.cc:2199: __ ldr(ip, FieldMemOperand(reg,
> HeapObject::kMapOffset));
> ip shouldn't be used outside the macro assembler. CompareRoot below will use
ip
> as a temp register to do the compare so in the end you will always branch (or
> assert in debug).
> 
> Lithium has a dedicated scratch register: scratch0() which you can use
instead.

ok, thanks for the helpful dbc

[oliv, 2013-06-24 09:11:00]

On 2013/06/21 16:38:04, danno wrote:
>
https://codereview.chromium.org/17229005/diff/23028/src/arm/lithium-codegen-a...
> src/arm/lithium-codegen-arm.cc:2164: int left_block =
> instr->TrueDestination(chunk_);
> Why this unrelated change?

neurosis?
it bothered me, that the definition of the variables did have the opposite order
than the method arguments :)

> Try to move this out with the other representation changes so that this code
can
> be shared 100% with hydrogen.

ok

>
https://codereview.chromium.org/17229005/diff/23028/src/code-stubs.cc#newcode227
> src/code-stubs.cc:227: void UnaryOpStub::Record(Handle<Object> object) {
> UpdateState?

better indeed

PTAL

[danno, 2013-06-27 13:31:06]

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:756: Handle<Code> UnaryOpStub::GenerateCode() {
Move this down after the BuildCode method for UnaryOpStub

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:761: HInstruction*
CodeStubGraphBuilderBase::BuildUnaryOp(HValue* input,
I wouldn't put this on the BuilderBase. If there is really no way to share this
code with the OptimizedGraphBuilder, then just make it a static method.

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:798: HInstruction* res = BuildUnaryOp(input, type,
stub->operation());
Yeah, I know it's a generated if, but I don't think indentation should be
changed (here and below)

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:812: Handle<Code> CompareNilICStub::GenerateCode() {
Move this up to be under the CompareNil BuildCode method

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs.h#newcode601
src/code-stubs.h:601: class OperatorBits      : public BitField<byte,
NUMBER_OF_TYPES, 1> {};
This isn't right. The NUMBER_OF_TYPES doesn't directly have to do with the bit
you use, and I don't think there is an easy way to couple the two.
NUMBER_OF_TYPES is 2 but that's not a bit offset. There's also no This should
be:

class OperatorBits         : public BitField<Token, 0, 8> {};
class StateEnumSetBits  : public BitField<int, 8, 3> {};

And the rest of the code should use those.

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4073: 
Remove this extraneous whitespace change

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen.h#newcode1019
src/hydrogen.h:1019: static Representation ToRepresentation(Handle<Type> type);
Shouldn't this method be on the Type class?

https://codereview.chromium.org/17229005/diff/116001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/17229005/diff/116001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:1611: ASSERT(instr->right()->IsConstant() &&
Shouldn't you use BetterRightOperand here and on the line below?

[oliv, 2013-06-27 17:20:47]

PTAL

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:756: Handle<Code> UnaryOpStub::GenerateCode() {
On 2013/06/27 13:31:06, danno wrote:
> Move this down after the BuildCode method for UnaryOpStub

Done.

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:761: HInstruction*
CodeStubGraphBuilderBase::BuildUnaryOp(HValue* input,
On 2013/06/27 13:31:06, danno wrote:
> I wouldn't put this on the BuilderBase. If there is really no way to share
this
> code with the OptimizedGraphBuilder, then just make it a static method.

Done.

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:798: HInstruction* res = BuildUnaryOp(input, type,
stub->operation());
On 2013/06/27 13:31:06, danno wrote:
> Yeah, I know it's a generated if, but I don't think indentation should be
> changed (here and below)

Done.

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:812: Handle<Code> CompareNilICStub::GenerateCode() {
On 2013/06/27 13:31:06, danno wrote:
> Move this up to be under the CompareNil BuildCode method

Done.

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/code-stubs.h#newcode601
src/code-stubs.h:601: class OperatorBits      : public BitField<byte,
NUMBER_OF_TYPES, 1> {};
On 2013/06/27 13:31:06, danno wrote:
> This isn't right. The NUMBER_OF_TYPES doesn't directly have to do 
> with the bit
> you use, and I don't think there is an easy way to couple the two.
> NUMBER_OF_TYPES is 2 but that's not a bit offset. There's also no 
> This should be:
> 
> class OperatorBits         : public BitField<Token, 0, 8> {};
> class StateEnumSetBits  : public BitField<int, 8, 3> {};
> 
> And the rest of the code should use those.

I think the code is correct. The encoding of the state is an enum set, which
uses one bit per type, thus the NUMBER_OF_TYPES (=3) is indeed the number of
bits, the enum set uses. The operator is either minus or not, thus i use just
one bit to encode it. This operater bit comes after the state, thus it has an
offset of NUMBER_OF_TYPES to the left.

I didn't use the BitField class to store the operator, since i only ever use 2
tokens of all the possible 8...

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4073: 
On 2013/06/27 13:31:06, danno wrote:
> Remove this extraneous whitespace change

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/17229005/diff/116001/src/hydrogen.h#newcode1019
src/hydrogen.h:1019: static Representation ToRepresentation(Handle<Type> type);
On 2013/06/27 13:31:06, danno wrote:
> Shouldn't this method be on the Type class?
I only pulled this method up from the OptimizedGraphBuilder, and its only used
in this hierarchy.

https://codereview.chromium.org/17229005/diff/116001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/17229005/diff/116001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:1611: ASSERT(instr->right()->IsConstant() &&
On 2013/06/27 13:31:06, danno wrote:
> Shouldn't you use BetterRightOperand here and on the line below?

Actually not, since i know the stub passes the constant as right operand. But i
should definitely not use betterLeftOperand, since that might be the constant...

[oliv, 2013-07-01 11:48:05]

PTAL, As discussed offline patchset 5 adds another BitField for the State and
stores the operation as a Token::Value.

I did however not remove the State EnumSet, since GetExtraIcState() needs to be
able to produce the ic_state without having access to the isolate, therefore we
cannot use a Type to represent the state. But at least its private now...

Additionally patchset 6 adds a small fix for the extra_ic_state in disassemble
output of code objects and reverts the changes to printing, since by now the
name added to the disassemble output contains the same information thus the
patch is not needed anymore.

[danno, 2013-07-02 12:03:09]

Almost there

https://codereview.chromium.org/17229005/diff/163003/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/17229005/diff/163003/src/hydrogen.h#newcode1019
src/hydrogen.h:1019: static Representation ToRepresentation(Handle<Type> type);
I think it makes sense to move ToRepresentation(Handle<Type> type) to
property_details.h where as a static helper function on Representation, e.g.
FromType(). There is no reason it should be limited to HGraphBuilder.

[oliv, 2013-07-02 13:12:33]

https://codereview.chromium.org/17229005/diff/163003/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/17229005/diff/163003/src/hydrogen.h#newcode1019
src/hydrogen.h:1019: static Representation ToRepresentation(Handle<Type> type);
On 2013/07/02 12:03:09, danno wrote:
> I think it makes sense to move ToRepresentation(Handle<Type> type) 
> to property_details.h where as a static helper function on 
> Representation, e.g.
> FromType(). There is no reason it should be limited to 
> HGraphBuilder.

ok, right that is a better place to stick the method!

[danno, 2013-07-03 08:32:42]

lgtm

[oliv, 2013-07-03 14:57:11]

Committed patchset #8 manually as r15473 (presubmit successful).