Last Comment Bug 723359 - IonMonkey: order of compares is incorrect on ARM
: IonMonkey: order of compares is incorrect on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 17:06 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-02-17 19:49 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/fixMA-r0.patch (6.04 KB, patch)
2012-02-01 17:06 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review
/home/mrosenberg/patches/fix_more_compares-r0.patch (3.68 KB, patch)
2012-02-06 13:22 PST, Marty Rosenberg [:mjrosenb]
jdemooij: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-02-01 17:06:27 PST
Created attachment 593662 [details] [diff] [review]
/home/mrosenberg/patches/fixMA-r0.patch

I noticed when boundsCheck was made indep, it broke due to the order of the arguments to cmp being swapped.  Looks like a handful of macro assembler functions had their arguments swapped.
Comment 1 Jacob Bramley [:jbramley] 2012-02-03 02:35:34 PST
Comment on attachment 593662 [details] [diff] [review]
/home/mrosenberg/patches/fixMA-r0.patch

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

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +529,4 @@
>          ma_b(label, cond);
>      }
>      void branchPtr(Condition cond, Register lhs, Register rhs, Label *label) {
> +        ma_cmp(lhs, rhs);

You swapped the order in ma_cmp too, so this one hasn't changed. Does that matter?
Comment 2 Marty Rosenberg [:mjrosenb] 2012-02-06 12:55:04 PST
That was the one that was backwards that made me realize the order was wrong elsewhere.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-02-06 13:22:41 PST
Created attachment 594784 [details] [diff] [review]
/home/mrosenberg/patches/fix_more_compares-r0.patch

It looks like after I wrote the previous patch, some new backwards uses were added, as well as a spurious breakpoint()! I've included the code that I used to track down the spurious BKPT, since I'm imagining it can be useful in the future.  That code can be trivially broken out into a separate patch if anyone wants.
Comment 4 Jan de Mooij [:jandem] 2012-02-06 14:37:51 PST
Comment on attachment 594784 [details] [diff] [review]
/home/mrosenberg/patches/fix_more_compares-r0.patch

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

::: js/src/ion/arm/Assembler-arm.cpp
@@ +1871,5 @@
> +    static int hit = 0;
> +    if (stopBKPT == hit)
> +        dbg_break();
> +    writeInst(0xe1200070 | (hit & 0xf) | ((hit & 0xfff0)<<4));
> +    hit++;

bkpt will only be used for debugging so this seems fine. However, I had to look up why the immediate was used so a small comment would be helpful. Probably just something like:

// The 16-bit immediate will be ignored by the processor, we use it here
// to track down masm.breakpoint() calls.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +298,5 @@
>      {
>      }
>      const PoolInfo & getInfo(int x) const {
>          static const PoolInfo nil = {0,0,0};
> +        if ((x < 0) || (x >= numDumps))

Nit: SM style is to not add these parentheses.

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