Last Comment Bug 766713 - IonMonkey: (ARM) LoadValue does not handle all of the cases that it should.
: IonMonkey: (ARM) LoadValue does not handle all of the cases that it should.
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
-- normal (vote)
: ---
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-06-20 14:13 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-06-21 19:52 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

/home/mrosenberg/patches/improveLoadValue-r0.patch (2.93 KB, patch)
2012-06-20 14:13 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description User image Marty Rosenberg [:mjrosenb] 2012-06-20 14:13:46 PDT
Created attachment 635062 [details] [diff] [review]

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.
Comment 1 User image Jacob Bramley [:jbramley] 2012-06-21 01:33:42 PDT
Comment on attachment 635062 [details] [diff] [review]

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.

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

  add     ScratchRegister, base, index<<TimesEight
  ldrd    val.payloadReg(), val.typeReg(), [ScratchRegister]
Comment 2 User image Marty Rosenberg [:mjrosenb] 2012-06-21 19:52:31 PDT

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