698695 - IonMonkey: make ARM pass jit-tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: Make --ion pass jit-tests

Some minor fixes to previous patches (and adds fixes needed from rebasing)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 570946 [details] [diff] [review]
Make --ion pass jit-tests

>--- a/js/src/assembler/assembler/MacroAssemblerARM.h
>+++ b/js/src/assembler/assembler/MacroAssemblerARM.h
>@@ -1,10 +1,10 @@
> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>- * vim: set ts=8 sw=4 et tw=79:
>+n * vim: set ts=8 sw=4 et tw=79:

I don't think you wanted this change.

Comment 2

[Jacob Bramley [:jbramley]]

Comment on attachment 570946 [details] [diff] [review]
Make --ion pass jit-tests

Review of attachment 570946 [details] [diff] [review]:
-----------------------------------------------------------------

Minor comments only. Please fix before pushing, but otherwise this has r+.

::: js/src/assembler/assembler/MacroAssemblerARM.h
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +n * vim: set ts=8 sw=4 et tw=79:

Looks like an error.

::: js/src/ion/IonMacroAssembler.cpp
@@ +145,5 @@
>  {
>      JS_ASSERT(inCall_);
>  
>      // Perform argument move resolution now.
> +#ifndef JS_CPU_ARM

What's this for? Doesn't ARM have stackAdjust_? If not, why not?

If we can't avoid the #ifndef here, it would probably be better to put the opening curly bracket on its own line so you only need one #ifndef block, like this:

#ifndef JS_CPU_ARM
    if (stackAdjust ... )
#endif
    {
        ...
    }

That would be much more readable, in my opinion.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +600,5 @@
>            }
>              break;
>          case JSOP_RSH:
>            if (rhs->isConstant()) {
> +              if (ToInt32(rhs) != 0) {

ToInt32(rhs)&0x1f != 0

@@ +612,5 @@
>              break;
>          case JSOP_URSH: {
>              MUrsh *ursh = ins->mir()->toUrsh();
>              if (rhs->isConstant()) {
> +                if (ToInt32(rhs) != 0) {

ToInt32(rhs)&0x1f != 0

::: js/src/ion/arm/LIR-arm.h
@@ +113,5 @@
>          setOperand(0, cindex);
>      }
>  };
>  
> +// just because we need to make a function call (and trash two argument registers + lr)

Actually you trash all four argument registers (r0-r3) and ip, as well as lr. Even if you don't use them for arguments, the target function will probably use them for temporaries and suchlike. r0-r3, ip and lr are caller-preserved.

@@ +123,5 @@
>      LDivI(const LAllocation &lhs, const LAllocation &rhs,
> +          const LDefinition &temp1, const LDefinition &temp2
> +#if 0
> +          , const LDefinition &temp3
> +#endif

Did you mean to leave that in?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +926,5 @@
>      ma_vcmp(lhs, rhs);
>      as_vmrs(pc);
>      switch (compare) {
>        case JSOP_LT:
> +        as_cmp(ScratchRegister, O2Reg(ScratchRegister), Overflow);

I don't understand what these cmp instructions are for. You do the VCMP, move the flags into APSR (with VMRS), then stamp on them with as_cmp. Why? This function was right before, wasn't it?

Comment 3

[Marty Rosenberg [:mjrosenb]]

(In reply to Jacob Bramley [:jbramley] from comment #2)

> 
> ::: js/src/ion/IonMacroAssembler.cpp
> @@ +145,5 @@
> >  {
> >      JS_ASSERT(inCall_);
> >  
> >      // Perform argument move resolution now.
> > +#ifndef JS_CPU_ARM
> 
> What's this for? Doesn't ARM have stackAdjust_? If not, why not?
> 
> If we can't avoid the #ifndef here, it would probably be better to put the
> opening curly bracket on its own line so you only need one #ifndef block,
> like this:
> 
> #ifndef JS_CPU_ARM
>     if (stackAdjust ... )
> #endif
>     {
>         ...
>     }
> 
> That would be much more readable, in my opinion.
> 
I've talked to nperrion about this already.  The problem is that we didn't run the move resolver/emitter
if there was nothing pushed onto the stack, so if all of the arguments fit into registers, we wouldn't actuall
assign anything.  The solution is going to be to just kill the check for everyone.

> ::: js/src/ion/arm/CodeGenerator-arm.cpp
> @@ +600,5 @@
> >            }
> >              break;
> >          case JSOP_RSH:
> >            if (rhs->isConstant()) {
> > +              if (ToInt32(rhs) != 0) {
> 
> ToInt32(rhs)&0x1f != 0
> 
> @@ +612,5 @@
> >              break;
> >          case JSOP_URSH: {
> >              MUrsh *ursh = ins->mir()->toUrsh();
> >              if (rhs->isConstant()) {
> > +                if (ToInt32(rhs) != 0) {
> 
> ToInt32(rhs)&0x1f != 0
> 
> ::: js/src/ion/arm/LIR-arm.h
> @@ +113,5 @@
> >          setOperand(0, cindex);
> >      }
> >  };
> >  
> > +// just because we need to make a function call (and trash two argument registers + lr)
> 
> Actually you trash all four argument registers (r0-r3) and ip, as well as
> lr. Even if you don't use them for arguments, the target function will
> probably use them for temporaries and suchlike. r0-r3, ip and lr are
> caller-preserved.
>
r0 and r1 are marked as copy, so they can be modified. r2 and r3 are marked as trashed, so
the function can use them.
 
> @@ +123,5 @@
> >      LDivI(const LAllocation &lhs, const LAllocation &rhs,
> > +          const LDefinition &temp1, const LDefinition &temp2
> > +#if 0
> > +          , const LDefinition &temp3
> > +#endif
> 
> Did you mean to leave that in?
>
Sort of.  Currently lr is marked as unallocatable.  Eventually, I'd like to be able to allocate to it, and when that happens, I'll need to inform the register allocator that lr is going to be trashed.
Until then, the greedy allocator breaks when you ask it for a temp in a register that can't be allocated.

> ::: js/src/ion/arm/MacroAssembler-arm.cpp
> @@ +926,5 @@
> >      ma_vcmp(lhs, rhs);
> >      as_vmrs(pc);
> >      switch (compare) {
> >        case JSOP_LT:
> > +        as_cmp(ScratchRegister, O2Reg(ScratchRegister), Overflow);
> 
> I don't understand what these cmp instructions are for. You do the VCMP,
> move the flags into APSR (with VMRS), then stamp on them with as_cmp. Why?
> This function was right before, wasn't it?

I copied this trick from the old assembler.  We only stop over them when the overflow bit is set, which is set when one of the arguments is a NaN.  This makes comparisons with NaN behave the way they should in JS.