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.h#newcode3232
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.h#newcode5689
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-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode106
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-ia32.cc#newcode374
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-ia32.cc#newcode561
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-ia32.cc#newcode582
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-ia32.cc#newcode2790
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-ia32.cc#newcode3217
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-ia32.cc#newcode4292
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-ia32.cc#newcode4533
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-ia32.cc#newcode4547
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#newcode2227
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#newcode2274
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#newcode2381
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#newcode2520
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#newcode2560
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#newcode2241
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.
https://codereview.chromium.org/13426006/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
