Closed Bug 677339 Opened 13 years ago Closed 13 years ago

IonMonkey: Implement conversion operations

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 1 obsolete file)

Script which currently fails:

function f(y)
{
    var x = 23.5;
    return x & y;
}
assertEq(f(93), 21);
Morphing - this has turned into a larger patch.
Summary: IonMonkey: Implement MTruncateToInt32 → IonMonkey: Implement conversion operations
Attached patch wip v0 (obsolete) — Splinter Review
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)).
MToDouble, MTruncateToInt32 will be in follow-up patches.
Attachment #552585 - Attachment is obsolete: true
Attachment #552611 - Flags: review?(sstangl)
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+
(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.
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+
Blocks: 678602
No longer blocks: 677337
Blocks: 670628
No longer blocks: 678602
Blocks: 677337
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.