Last Comment Bug 714201 - IonMonkey: JSOP_MOD support for ARM
: IonMonkey: JSOP_MOD support for ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 12 Branch
: ARM Linux
: -- normal (vote)
: ---
Assigned To: Eddy Bruel [:ejpbruel]
:
Mentors:
Depends on: 707927
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-29 17:10 PST by Sean Stangl [:sstangl]
Modified: 2012-02-17 19:47 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
c+p of LDivI (6.99 KB, patch)
2012-02-01 16:59 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Review

Description Sean Stangl [:sstangl] 2011-12-29 17:10:15 PST
We need integer JSOP_MOD support on ARM to run the crypto-md5 benchmark.
Comment 1 Eddy Bruel [:ejpbruel] 2011-12-30 06:06:50 PST
Since I've implements JSOP_MOD on x86 and x64, and would like to learn about ARM, I hope it's ok if I take this one as well?
Comment 2 Marty Rosenberg [:mjrosenb] 2011-12-30 13:48:19 PST
Go for it.  Just a word or two of warning:
JSOP_DIV on arm is just a call to a glibc function.  This function may or may not also store the
modulus
The call to the glibc function is unchecked.  X/0 will throw an assertion, and INT_MIN/-1 will do something that we don't want it to do (return INT_MIN, I believe)
Comment 3 Marty Rosenberg [:mjrosenb] 2012-02-01 16:59:07 PST
Created attachment 593659 [details] [diff] [review]
c+p of LDivI

This at least implements the integer part of JSOP_MOD for arm
Comment 4 Jacob Bramley [:jbramley] 2012-02-03 02:28:42 PST
Comment on attachment 593659 [details] [diff] [review]
c+p of LDivI

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

There's a problem with __aeabi_idiv, but the fix is trivial. With that fix, the patch is fine.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +504,5 @@
>      return true;
>  }
>  
>  bool
> +CodeGeneratorARM::visitModI(LModI *ins)

__aeabi_idiv _almost_ returns the modulo in r3, but it doesn't have the correct sign in some cases. Specifically, the relationship "dividend = quotient * divisor + remainder" does not hold if the dividend is negative. Instead, you should use __aeabi_idivmod (returning the quotient in r0 and the remainder in r1). Results from a simple C test:

C operators:
   7/ 3 =  2,  7/ 3 =  1         7 = (2 * 3) + 1
  -7/ 3 = -2, -7/ 3 = -1         -7 = (-2 * 3) + -1
   7/-3 = -2,  7/-3 =  1         7 = (-2 * -3) + 1
  -7/-3 =  2, -7/-3 = -1         -7 = (2 * -3) + -1
__aeabi_idiv:
   7/ 3 =  2,  7/ 3 =  1         7 = (2 * 3) + 1
  -7/ 3 = -2, -7/ 3 =  1  ERROR: -7 != (-2 * 3) +  1
   7/-3 = -2,  7/-3 =  1         7 = (-2 * -3) + 1
  -7/-3 =  2, -7/-3 =  1  ERROR: -7 != (2 * -3) +  1
__aeabi_idivmod:
   7/ 3 =  2,  7/ 3 =  1         7 = (2 * 3) + 1
  -7/ 3 = -2, -7/ 3 = -1         -7 = (-2 * 3) + -1
   7/-3 = -2,  7/-3 =  1         7 = (-2 * -3) + 1
  -7/-3 =  2, -7/-3 = -1         -7 = (2 * -3) + -1

Note that __aeabi_idiv seems to be fine for visitDivI, where the sign of the remainder is not important.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +851,5 @@
>  
>  RelocBranchStyle
>  b_type()
>  {
> +    return MOVWT;

This change looks unrelated.

Note You need to log in before you can comment on or make changes to this bug.