The default bug view has changed. See this FAQ.

IonMonkey: (ARM) LoadValue does not handle all of the cases that it should.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 635062 [details] [diff] [review]
/home/mrosenberg/patches/improveLoadValue-r0.patch

This is for the case that loadvalue is given a BaseIndex, which is for operations
of the form
x = *(ra + rb << c + #d)
When this code was written, it was assumed that #d would always be 0, but since then, that invariant has varied.  Simple fix for the issue is attached.
Attachment #635062 - Flags: review?(Jacob.Bramley)
Comment on attachment 635062 [details] [diff] [review]
/home/mrosenberg/patches/improveLoadValue-r0.patch

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

The implementation is correct, as far as I can see. I suggested a simplification, but the patch is good whether you choose to do that or not.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2383,1 @@
>      }

Can't the whole thing be reduced in the same way? The simpler loadValue will try to use ldrd anyway, so you only need two lines for the whole function, I think:

ma_alu(base, lsl(index, TimesEight), ScratchRegister, op_add);
loadValue(Address(ScratchRegister, off.value), val);

The only difference is where the address calculation is done in the case where off.value is 0.

Now:
  lsl     ScratchRegister, index, TimesEight
  ldrd    val.payloadReg(), val.typeReg(), [base, ScratchRegister]

Simplified:
  add     ScratchRegister, base, index<<TimesEight
  ldrd    val.payloadReg(), val.typeReg(), [ScratchRegister]
Attachment #635062 - Flags: review?(Jacob.Bramley) → review+
(Reporter)

Comment 2

5 years ago
landed: http://hg.mozilla.org/projects/ionmonkey/rev/c8ce2a0acddb
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.