Improvements for x87 stack handling

Improvements for x87 stack handling

BUG=

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

[mvstanton, 2013-04-08 12:17:01]

Hi Danno,
Here is the x87 work. There is a TODO in EmitNumberUntagD for -0.0 detection,
I'm looking at that.

Thanks,
--Michael

[danno, 2013-04-08 12:57:27]

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

https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3232: return !representation().IsDouble() ||
ImmortalImmovable();
Maybe it's better to factor our the special cases for doubles into another
predicate, like IsSpecialDouble() that is used both here and in
ImmortalImmovable.

https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5689:
HConstant::cast(value())->IsDoubleCanonicalHole();
This predicate seems misleading... it checks only for the double canonical hold,
but not the real hole. Either rename the predicate or add support for testing
for the the_hole constant tagged value.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:374: if (instr->ClobbersDoubleRegisters()) {
I am not sure that is safe to do _after_ the CompileToNative. What if instr is a
call? That could call other JavaScript code that manipulates the FP stack, and
there would be something on the stack. I think this should be before
CompileToNative, checking instr to see if any of its inputs are FP tos. If not,
then push, then if the instruction is ClobbersDoubleRegisters or
HadDoubleRegisterResult, then it should PopX87 if the stack isn't empty.

This strategy would also let you properly handle Gaps... if you never "pop" from
a gap, then this code makes sure that the pop happens before the next
instruction, or not at all if the next instruction uses the value.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:561: void LCodeGen::PushX87Operand(Operand src)
{
Maybe make this PushX87DoubleOperand?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:582: void LCodeGen::MarkReturnX87Result() {
Maybe CurrentInstructionReturnsX87Result?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2790: __ SmiUntag(reg);  // it is a smi
Is this an unrelated bug?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:3217: }
Unrelated changes?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4292: if (instr->value()->IsConstantOperand())
{
Since this has now gotten bigger, can you merge this in- and out- of line
property case? i.e. do the "__ mov(temp, FieldOperand(object,
JSObject::kPropertiesOffset))" first, then swap temp and object registers? Here
and on other platforms.

Also... is this change related? It's a good change.... but maybe not here? :-)

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4533: if (instr->value()->IsRegister()) {
Is this change related to SSE2 at all?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4547: ASSERT(instr->value()->IsRegister());
Is this change related to SSE2 at all?

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

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2227: LOperand*
LChunkBuilder::GetStoreKeyedValueOperand(HStoreKeyed* instr) {
Is this change related?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2381: : UseRegisterOrConstant(instr->value());
Is this change related?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2520: Register reg;
unrelated change?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2560: LOperand* length;
unrelated change?

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

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:1408: DECLARE_HYDROGEN_ACCESSOR(Return)
Unrelated change?

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:2893: LOperand* GetStoreKeyedValueOperand(HStoreKeyed*
instr);
Is this change related?

[mvstanton, 2013-04-08 16:10:14]

Hi Danno,
Updates applied. Tests including a noenable_sse2 run successful. I kept the code
in the gap allocator very explicit regarding the stack. Also the logic remains
very explicit in the GenerateBody() method when deciding to pop the x87 stack.

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

https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3232: return !representation().IsDouble() ||
ImmortalImmovable();
On 2013/04/08 12:57:27, danno wrote:
> Maybe it's better to factor our the special cases for doubles into another
> predicate, like IsSpecialDouble() that is used both here and in
> ImmortalImmovable.

I did this kind of refactoring but more in the methods ImmortalImmovable(), and
then below with the new IsTheHole() method. Does that correspond?

https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5689:
HConstant::cast(value())->IsDoubleCanonicalHole();
On 2013/04/08 12:57:27, danno wrote:
> This predicate seems misleading... it checks only for the double canonical
hold,
> but not the real hole. Either rename the predicate or add support for testing
> for the the_hole constant tagged value.

Done.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:106: code->kind() == Code::OPTIMIZED_FUNCTION)
{
I don't believe this is needed for this CL.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:374: if (instr->ClobbersDoubleRegisters()) {
On 2013/04/08 12:57:27, danno wrote:
> I am not sure that is safe to do _after_ the CompileToNative. What if instr is
a
> call? That could call other JavaScript code that manipulates the FP stack, and
> there would be something on the stack. I think this should be before
> CompileToNative, checking instr to see if any of its inputs are FP tos. If
not,
> then push, then if the instruction is ClobbersDoubleRegisters or
> HadDoubleRegisterResult, then it should PopX87 if the stack isn't empty.
> 
> This strategy would also let you properly handle Gaps... if you never "pop"
from
> a gap, then this code makes sure that the pop happens before the next
> instruction, or not at all if the next instruction uses the value.

Thanks for your extensive help with this brain teaser. Updates applied.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:561: void LCodeGen::PushX87Operand(Operand src)
{
On 2013/04/08 12:57:27, danno wrote:
> Maybe make this PushX87DoubleOperand?

Done.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:582: void LCodeGen::MarkReturnX87Result() {
On 2013/04/08 12:57:27, danno wrote:
> Maybe CurrentInstructionReturnsX87Result?

Done.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2790: __ SmiUntag(reg);  // it is a smi
On 2013/04/08 12:57:27, danno wrote:
> Is this an unrelated bug?
Yep, removed.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:3217: }
On 2013/04/08 12:57:27, danno wrote:
> Unrelated changes?
Yep, removed.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4292: if (instr->value()->IsConstantOperand())
{
On 2013/04/08 12:57:27, danno wrote:
> Since this has now gotten bigger, can you merge this in- and out- of line
> property case? i.e. do the "__ mov(temp, FieldOperand(object,
> JSObject::kPropertiesOffset))" first, then swap temp and object registers?
Here
> and on other platforms.
> 
> Also... is this change related? It's a good change.... but maybe not here? :-)


All unrelated, removed!

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4533: if (instr->value()->IsRegister()) {
On 2013/04/08 12:57:27, danno wrote:
> Is this change related to SSE2 at all?

Nope, removed.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4547: ASSERT(instr->value()->IsRegister());
On 2013/04/08 12:57:27, danno wrote:
> Is this change related to SSE2 at all?
Nosir, removed.

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

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2227: LOperand*
LChunkBuilder::GetStoreKeyedValueOperand(HStoreKeyed* instr) {
On 2013/04/08 12:57:27, danno wrote:
> Is this change related?
Nope, removed

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2274: val = UseTempRegister(instr->value());
This change is unrelated, removing.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2381: : UseRegisterOrConstant(instr->value());
On 2013/04/08 12:57:27, danno wrote:
> Is this change related?

Nope, removed.

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2520: Register reg;
On 2013/04/08 12:57:27, danno wrote:
> unrelated change?
Yep, removed

https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:2560: LOperand* length;
On 2013/04/08 12:57:27, danno wrote:
> unrelated change?
Indeed, thx.

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

https://codereview.chromium.org/13426006/diff/19011/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2241: // Determine if we need a byte register in this
case for the value.
Per the register allocator issue we saw, I kept this change from you in.

[danno, 2013-04-08 20:51:20]

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

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3194: ASSERT(has_double_value_);
Any reason this has to be an assert? Why not just return "false" if
has_double_value_ is false?

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3237: return !representation().IsDouble() ||
ImmortalImmovable();
|| IsSpecialDouble instead of || ImmortanImmovable?

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3343: return
FixedDoubleArray::is_the_hole_nan(double_value_);
Since this is only used once, it's probably fine just to inline
FixedDoubleArray::is_the_hole_nan(double_value_) in "IsTheHole"

[mvstanton, 2013-04-09 07:13:08]

Thanks, Danno, comments applied.

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

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3194: ASSERT(has_double_value_);
On 2013/04/08 20:51:21, danno wrote:
> Any reason this has to be an assert? Why not just return "false" if
> has_double_value_ is false?
Good idea, done.

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3237: return !representation().IsDouble() ||
ImmortalImmovable();
On 2013/04/08 20:51:21, danno wrote:
> || IsSpecialDouble instead of || ImmortanImmovable?

Now I see, thanks.

https://codereview.chromium.org/13426006/diff/19011/src/hydrogen-instructions...
src/hydrogen-instructions.h:3343: return
FixedDoubleArray::is_the_hole_nan(double_value_);
On 2013/04/08 20:51:21, danno wrote:
> Since this is only used once, it's probably fine just to inline
> FixedDoubleArray::is_the_hole_nan(double_value_) in "IsTheHole"

Also in IsSpecialDouble(), but that is okay, this method is awkward.

[danno, 2013-04-09 07:37:07]

lgtm with comments

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

https://codereview.chromium.org/13426006/diff/26001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3204: 
nit: remove added whitespace

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

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:370: if (!CpuFeatures::IsSupported(SSE2)) {
nit: How about putting the following 7 lines in a utility function called
"FlushX87StackIfNecessary?"

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:386: // Make sure the floating point stack is
either empty or has one item,
This code might be useful elsewhere. How about putting it into the
macro-assembler in a function called "CheckX87StackIsEmpty()"

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-gap-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:378: cgen_->PopX87();
Slightly faster might be to just store rather that pop and store with a push...
but it's probably not important.

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

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2270: } else {
nit: no need for the additional scope, just do a "else if"

[mvstanton, 2013-04-09 08:43:13]

Committed patchset #6 manually as r14179 (presubmit successful).

[mvstanton, 2013-04-09 08:49:13]

Committed, thx for the review!

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

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:370: if (!CpuFeatures::IsSupported(SSE2)) {
On 2013/04/09 07:37:07, danno wrote:
> nit: How about putting the following 7 lines in a utility function called
> "FlushX87StackIfNecessary?"

Done.

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:386: // Make sure the floating point stack is
either empty or has one item,
On 2013/04/09 07:37:07, danno wrote:
> This code might be useful elsewhere. How about putting it into the
> macro-assembler in a function called "CheckX87StackIsEmpty()"

Done with a variant: I also check if the stack has one item, so I generalized it
to VerifyX87StackDepth(uint depth), that way we can verify any expected depth.

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-gap-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:378: cgen_->PopX87();
On 2013/04/09 07:37:07, danno wrote:
> Slightly faster might be to just store rather that pop and store with a
push...
> but it's probably not important.

Hmm, I'm looking at the x87 instructions, and all variants of the fld
instruction do something like a push, so I'd end up with two items on the stack.
I don't see an "overwrite" instruction or mode.

But agreed, this area will look different with N slots to play with instead of
just 1.

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

https://codereview.chromium.org/13426006/diff/26001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2270: } else {
On 2013/04/09 07:37:07, danno wrote:
> nit: no need for the additional scope, just do a "else if"

Done.