IonMonkey: (ARM) Add support for hardware divide instructions

RESOLVED FIXED in mozilla25



6 years ago
6 years ago


(Reporter: mjrosenb, Assigned: jonco)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

16.49 KB, patch
: review+
Details | Diff | Splinter Review
Cortex-A15 and newer processors have a hardware divide instruction that we should generate when they are available. The arm chromebooks have the A15, so they should be usable for testing this code.
Assignee: general → jcoppeard
Posted patch v0 (obsolete) — Splinter Review
Here's what I've got so far.  This makes use of SDIV when its supported and passes the jit tests and jstests on my chromebook.

Unlike x86 we don't get a remainder from the divide instruction, so if the result is not
truncated I multiply it up again to check whether we need to bail out.

Let me know what you think.  There's a couple of things I think that could be

 - the LDivI always takes its arguments in r0 and r1, and also uses r2 and r3, which
   is no longer necessary if we're going to use the divide instruction.  We could have a
   separate constructor for LDivI that doesn't do this.

 - when multiplying up the result, I don't corrupt the input registers.  I'm not
   sure whether this is necessary for the bailout to work, or whether it gets its
   inputs from somewhere else.
Attachment #757906 - Flags: feedback?(mrosenberg)
Comment on attachment 757906 [details] [diff] [review]

Review of attachment 757906 [details] [diff] [review]:

Other than the choice to make the instruction/call decision in the code generator rather than lowering, it looks good.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +554,5 @@
>                  return false;
>          }
>      }
> +
> +    if (hasIDIV()) {

I'd recommend doing this check in the lowering phase rather than the code generator. This way, you can only allocate the registers that are actually needed.  the call-full divide takes 4 registers, and hardcodes r0-r3, since they are used by the call.
Attachment #757906 - Flags: feedback?(mrosenberg) → feedback+
Posted patch Patch v2Splinter Review
It took me ages to get round to this, but here's the updated patch.
Attachment #757906 - Attachment is obsolete: true
Attachment #765964 - Flags: review?(mrosenberg)
Comment on attachment 765964 [details] [diff] [review]
Patch v2

Review of attachment 765964 [details] [diff] [review]:

Everything looks good. A future improvement can be to use the hardware divide to calculate modulus as well.

::: js/src/ion/arm/LIR-arm.h
@@ +94,5 @@
> +        return mir_->toDiv();
> +    }
> +};
> +
> +// LSoftDivI is a software divide for architectures before ARMv7 that don't

most ARMv7 chips don't have hardware divide instructions
only the brand-spanking new cortex-A15 and compatible processors have hardware divide.
Attachment #765964 - Flags: review?(mrosenberg) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Duplicate of this bug: 888886
You need to log in before you can comment on or make changes to this bug.