Closed Bug 769363 Opened 12 years ago Closed 12 years ago

IonMonkey: LBoundsCheck should not require a register for the length operand

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(1 file)

On x86, we want to allow using a stack slot for LBoundsCheck's length operand to avoid a register (load) on am3 (and hoisting an array length is pretty common).
Assignee: general → evilpies
Attached patch v1Splinter Review
This passes try, but I haven't found time yet to figure out if it actually helps on x86.
Attachment #655367 - Flags: review?(jdemooij)
Comment on attachment 655367 [details] [diff] [review]
v1

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

Looks good, thanks!

::: js/src/ion/Lowering.cpp
@@ +1303,5 @@
>  {
>      LInstruction *check;
>      if (ins->minimum() || ins->maximum()) {
>          check = new LBoundsCheckRange(useRegisterOrConstant(ins->index()),
> +                                      useAnyOrConstant(ins->length()),

You should add useAny (similar to useAnyOrConstant) and call it here, since BoundsCheckRange codegen doesn't handle a constant length operand. Not sure if this case will actually end up here, but it's best to be safe.

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +101,5 @@
>      }
> +    void cmp32(const Operand &src, const Imm32 &imm) {
> +        cmpl(src, imm);
> +    }
> +    void cmp32(const Operand &lhs, const Register &rhs) {

Nit: please change the other cmp32 methods to also use lhs/rhs for the argument names.
Attachment #655367 - Flags: review?(jdemooij) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/2257377e7972
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This wasn't a very obvious win, but the patch is small.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.