The default bug view has changed. See this FAQ.

IonMonkey: JSOP_MOD support for ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sstangl, Assigned: ejpbruel)

Tracking

12 Branch
ARM
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
We need integer JSOP_MOD support on ARM to run the crypto-md5 benchmark.
(Assignee)

Comment 1

5 years ago
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?
Assignee: general → ejpbruel
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)
Created attachment 593659 [details] [diff] [review]
c+p of LDivI

This at least implements the integer part of JSOP_MOD for arm
Attachment #593659 - Flags: review?(Jacob.Bramley)
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.
Attachment #593659 - Flags: review?(Jacob.Bramley) → review+
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.