IonMonkey: order of compares is incorrect on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.
Attachment #593662 - Flags: review?(Jacob.Bramley)
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?
Attachment #593662 - Flags: review?(Jacob.Bramley) → review+
(Reporter)

Comment 2

6 years ago
That was the one that was backwards that made me realize the order was wrong elsewhere.
(Reporter)

Comment 3

6 years ago
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.
Attachment #594784 - Flags: review?(jdemooij)
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.
Attachment #594784 - Flags: review?(jdemooij) → review+
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.