938468 - Backtracking allocator: miscompile on ss-aes on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Attachment: IONFLAGS=regalloc output

The backtracking allocator miscompiles crypto-aes.js on ARM. Here is a reduced testcase:

var q = new Array(1);
function KeyExpansion() {
  var w = new Array(60);
  for (var i=0; i<8; i++)
    w[i] = q;
  var Nk = 8;
  for (; i<60; i++) {
    w[i] = q;
    for (var t=0; t<4; t++);
    if (i % Nk == 0)
      for (var t=0; t<3; t++);
    for (var t=0; t<3; t++) w[i-8][0];
  }
}
KeyExpansion();
KeyExpansion();

Running this with --ion-regalloc=backtracking produces the following error:

c.js:12:28 TypeError: w[(i - 8)] is undefined

which is bogus. I tested this on an ARM chip on which the % operator is translated as LSoftModI.

Attached is the IONFLAGS=regalloc output on this reduced testcase.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: backtracking allocator backend fixes

Comment 2

[Dan Gohman [:sunfish]]

Comment on attachment 832017 [details] [diff] [review]
backtracking allocator backend fixes

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

Awesome, thanks for fixing this!

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +757,5 @@
>      // Extract the registers from this instruction
>      Register lhs = ToRegister(ins->lhs());
>      Register rhs = ToRegister(ins->rhs());
>      Register output = ToRegister(ins->output());
> +    Register callTemp = ToRegister(ins->getTemp(3));

Please add an accessor to LSoftModI to return this temporary.

::: js/src/jit/arm/LIR-arm.h
@@ +145,5 @@
>  // encoded such that the divisor and dividend are passed in their apropriate
>  // registers, and are marked as copy so we can modify them (and the function
>  // will).  The other thre registers that can be trashed are marked as such. For
>  // the time being, the link register is not marked as trashed because we never
>  // allocate to the link register.

Please update this comment to better reflect the new reality in the code. Specifically, saying r0 and r1 are "marked as copy" isn't clear.

::: js/src/jit/arm/Lowering-arm.cpp
@@ +290,5 @@
>          return define(lir, div);
>      }
>  
> +    LSoftDivI *lir = new LSoftDivI(useFixedAtStart(div->lhs(), r0), useFixedAtStart(div->rhs(), r1),
> +                                   tempFixed(r1), tempFixed(r2), tempFixed(r3));

Is there some mechanism by which the call-clobbered float registers (d0 through d7 it appears) are declared as being clobbered here? Or are they not clobbered here?

Comment 3

[Dan Gohman [:sunfish]]

Nice, this patch also fixes the remaining mochitest and jsreftest failures. The backtracking allocator is completely green on ARM with this patch and the patch for bug 937944:

https://tbpl.mozilla.org/?tree=Try&rev=ecedbc2e7deb

Comment 4

[Douglas Crosher [:dougc]]

Attachment: backtracking allocator backend fixes

Address reviewer feedback.

Carrying forward r+

Comment 5

[Douglas Crosher [:dougc]]

(In reply to Dan Gohman [:sunfish] from comment #2)
> Comment on attachment 832017 [details] [diff] [review]
> backtracking allocator backend fixes
> 
> Review of attachment 832017 [details] [diff] [review]:
> -----------------------------------------------------------------

Thank you for the quick review and testing.


> ::: js/src/jit/arm/CodeGenerator-arm.cpp
> @@ +757,5 @@
> >      // Extract the registers from this instruction
> >      Register lhs = ToRegister(ins->lhs());
> >      Register rhs = ToRegister(ins->rhs());
> >      Register output = ToRegister(ins->output());
> > +    Register callTemp = ToRegister(ins->getTemp(3));
> 
> Please add an accessor to LSoftModI to return this temporary.

Done.

It appears that the callTemp is not needed when the instruction is truncated, but this can be explored in another bug.

> ::: js/src/jit/arm/LIR-arm.h
> @@ +145,5 @@
> >  // encoded such that the divisor and dividend are passed in their apropriate
> >  // registers, and are marked as copy so we can modify them (and the function
> >  // will).  The other thre registers that can be trashed are marked as such. For
> >  // the time being, the link register is not marked as trashed because we never
> >  // allocate to the link register.
> 
> Please update this comment to better reflect the new reality in the code.
> Specifically, saying r0 and r1 are "marked as copy" isn't clear.

Made an attempt to update the comments.
 
> ::: js/src/jit/arm/Lowering-arm.cpp
> @@ +290,5 @@
> >          return define(lir, div);
> >      }
> >  
> > +    LSoftDivI *lir = new LSoftDivI(useFixedAtStart(div->lhs(), r0), useFixedAtStart(div->rhs(), r1),
> > +                                   tempFixed(r1), tempFixed(r2), tempFixed(r3));
> 
> Is there some mechanism by which the call-clobbered float registers (d0
> through d7 it appears) are declared as being clobbered here? Or are they not
> clobbered here?

Even though the ABI considers some FP registers as caller saved, the function being called does not touch them and the FP registers are assumed to be preserved here. This seems reasonable given that these are primitive operations that demand top performance.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e5d950c21b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67e5d950c21b