Closed
Bug 677339
Opened 13 years ago
Closed 13 years ago
IonMonkey: Implement conversion operations
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(2 files, 1 obsolete file)
73.27 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
14.24 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
Script which currently fails: function f(y) { var x = 23.5; return x & y; } assertEq(f(93), 21);
Assignee | ||
Comment 1•13 years ago
|
||
Morphing - this has turned into a larger patch.
Summary: IonMonkey: Implement MTruncateToInt32 → IonMonkey: Implement conversion operations
Assignee | ||
Comment 2•13 years ago
|
||
This patch introduces x86 lowering and codegen for MToInt32. So far there are a few related refactorings: * Instructions start exposing named accessors instead of getOperand(0) etc. * All platforms are now required to reserve a scratch float register. * x64 now reserves a scratch general-purpose register. * Platform MacroAssemblers must now conform to a boxing and unboxing API. This will let us avoid a lot of duplication, allowing us to write generic LIR implementations that just work across all platforms. For example, we now don't have to implement ValueToInt32 three times. It won't be the most efficient thing on x64, but these ops are not the critical path (they're for expressions like (false + 5)).
Assignee | ||
Comment 3•13 years ago
|
||
MToDouble, MTruncateToInt32 will be in follow-up patches.
Attachment #552585 -
Attachment is obsolete: true
Attachment #552611 -
Flags: review?(sstangl)
Comment 4•13 years ago
|
||
Comment on attachment 552611 [details] [diff] [review] part 1: lower MToInt32 on x86, x64 Review of attachment 552611 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/assembler/assembler/X86Assembler.h @@ +291,4 @@ > } ThreeByteOpcodeID; > > + typedef enum { > + ESCAPE_PTEST = 0x38, Looking at http://software.intel.com/file/18187/ (page 195) , there appears to be no rhyme or reason behind the distinction of 0x38 versus 0x3A. I was hoping for more generic names. Oh well. ::: js/src/ion/CodeGenerator.cpp @@ +54,5 @@ > > bool > +CodeGenerator::visitValueToInt32(LValueToInt32 *lir) > +{ > + masm.breakpoint(); Segmentation fault. ::: js/src/ion/Lowering.cpp @@ +291,5 @@ > return false; > } > > +static void > +SpewSnapshot(MInstruction *ins, MSnapshot *snapshot) This is specific to the C1Visualizer-formatted output (I assume, since it's certainly invalid for JSON). Could this function be relocated to C1Spewer.cpp? ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +381,5 @@ > +CodeGeneratorX86Shared::emitDoubleToInt32(const FloatRegister &src, const Register &dest, LSnapshot *snapshot) > +{ > + // Note that we don't specify the destination width for the truncated > + // conversion to integer. x64 will use the native width (quadword) which > + // sign-extends the top bits, preserving a little sanity. A comment stating something to the effect of "Check whether double is representable as int32" can't hurt. ::: js/src/ion/shared/Lowering-shared-inl.h @@ +126,5 @@ > +IsCompatibleLIRCoercion(MIRType to, MIRType from) > +{ > + if (to == from) > + return true; > + if ((to == MIRType_Int32 || to == MIRType_Boolean) && return (to == ... || to == ...) && (from == ... ...); ::: js/src/ion/x64/Architecture-x64.h @@ +80,5 @@ > static const Code StackPointer = JSC::X86Registers::esp; > static const Code Invalid = JSC::X86Registers::invalid_reg; > > static const uint32 Total = 16; > + static const uint32 Allocatable = 14; %r11 was added to the NonAllocatableMask, but Allocatable went up by one! Should be 12, I think. ::: js/src/ion/x64/MacroAssembler-x64.h @@ +129,5 @@ > + // Type-testing instructions on x64 will clobber ScratchReg. > + Condition testInt32(Condition cond, const ValueOperand &src) { > + JS_ASSERT(cond == Equal || cond == NotEqual); > + splitTag(src.value(), ScratchReg); > + cmpl(ImmTag(JSVAL_TAG_INT32), ScratchReg); Hm, could we have a testTag(Tag, Reg) instead of hardcoding the behavior of splitTag in several places? ::: js/src/ion/x64/Trampoline-x64.cpp @@ +156,5 @@ > masm.subq(rsp, r14); > masm.push(r14); > > // Call function. > + masm.breakpoint(); Segmentation fault.
Attachment #552611 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Sean Stangl from comment #4) > Comment on attachment 552611 [details] [diff] [review] > Looking at http://software.intel.com/file/18187/ (page 195) , there appears > to be no rhyme or reason behind the distinction of 0x38 versus 0x3A. I was > hoping for more generic names. Oh well. Yeah I couldn't find what these actually meant either. > This is specific to the C1Visualizer-formatted output (I assume, since it's > certainly invalid for JSON). Could this function be relocated to > C1Spewer.cpp? It's actually just normal console spew. It wasn't related to this patch but I needed to debug some snapshot stuff. > > + if ((to == MIRType_Int32 || to == MIRType_Boolean) && > > return (to == ... || to == ...) && (from == ... ...); I prefer not to condense big expressions like this because it makes it harder to see which path it took in the debugger. > %r11 was added to the NonAllocatableMask, but Allocatable went up by one! > Should be 12, I think. That was a mistake on my part, the original value of 13 was wrong and should have been 15. > Hm, could we have a testTag(Tag, Reg) instead of hardcoding the behavior of > splitTag in several places? Yup, good idea.
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #552784 -
Flags: review?(sstangl)
Comment 7•13 years ago
|
||
Comment on attachment 552784 [details] [diff] [review] part 2: implement MToDouble Review of attachment 552784 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +114,5 @@ > + > + Assembler::Condition cond; > + Label isDouble, isInt32, isBool, isNull, done; > + > + masm.breakpoint(); :P ::: js/src/ion/Lowering.cpp @@ +261,5 @@ > + case MIRType_Int32: > + case MIRType_Boolean: > + { > + LInt32ToDouble *lir = new LInt32ToDouble(useRegister(opd)); > + return define(lir, convert) && assignSnapshot(lir); Without assignSnapshot(lir).
Attachment #552784 -
Flags: review?(sstangl) → review+
Updated•13 years ago
|
Updated•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•