723359 - IonMonkey: order of compares is incorrect on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /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]]

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]]

That was the one that was backwards that made me realize the order was wrong elsewhere.

Comment 3

[Marty Rosenberg [:mjrosenb]]

Attachment: /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]]

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.