779411 - IonMonkey: Regalloc double-assignation of useRegister() and tempFixed() at isCall().

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

The landing of Bug 772892 exposed a bug in register allocation: if an isCall() LInstruction takes an input via useRegister() and a temporary as useFixed(CallTempReg0), it can be the case that the register assigned to the operand is the same as the fixed temporary.

This was causing a crash in v8-v5/check-raytrace.js.

I added a hack to the tree to work around this issue for the moment ( http://hg.mozilla.org/projects/ionmonkey/rev/a73cf1325796 ). To reproduce, simply revert that patch by changing useFixed(power, CallTempReg1) to useRegister(power) in LIRGenerator::visitPow(). Running that test will then (sometimes!) trip the (power != temp) assertion in CodeGenerator::visitPowI(). Passing --no-jm may be required.

Comment 1

[Jan de Mooij [:jandem]]

Hm, good catch. The problem is that call uses are always implicitly at-start, but this is indeed confusing if there's also a temp.

We should either stop doing this and just assert all uses are fixed or at-start for call instructions (we used to do this) or assert there are no temps when we mark it as at-start.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Use at-start explicitly rather than implicitly for call instructions, passes jit-tests on 32-bit and 64-bit.

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 647891 [details] [diff] [review]
Patch

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

::: js/src/ion/LinearScan.cpp
@@ +599,5 @@
>  
> +                    // Call uses should always be at-start or fixed, since the fixed intervals
> +                    // use all registers.
> +                    JS_ASSERT_IF(ins->isCall() && !alloc.isSnapshotInput(),
> +                                 use->isFixedRegister() || use->usedAtStart());

We also need to assert that non-fixed GPRs are not used at the same time that a temporary is requested for a callsite. Otherwise this patch doesn't cause the obnoxious case to be detected and just adds more details for coders to remember.

@@ +609,5 @@
>                          if (!addFixedRangeAtHead(reg, inputOf(*ins), outputOf(*ins)))
>                              return false;
>                          to = inputOf(*ins);
>                      } else {
> +                        to = use->usedAtStart() ? inputOf(*ins) : outputOf(*ins);

This has different behavior than the previous code if isCall() && alloc.isSnapshotInput(). I'm not sure how that case can occur. If it can't, could an assertion be added?

::: js/src/ion/Lowering.cpp
@@ +682,5 @@
>  
>      if (power->type() == MIRType_Int32) {
> +        // Note: useRegisterAtStart here is safe, the temp is a GP register so
> +        // it will never get the same register.
> +        LPowI *lir = new LPowI(useRegisterAtStart(input), useFixed(power, CallTempReg1),

We want to be able to write useRegisterOrConstantAtStart(power) instead of useFixed(...) here. This patch still doesn't let us, but it should at least assert if we tried to.

::: js/src/ion/x86/Lowering-x86.cpp
@@ +28,5 @@
>  
>  bool
> +LIRGeneratorX86::useBoxAtStart(LInstruction *lir, size_t n, MDefinition *mir)
> +{
> +    return useBox(lir, n, mir, LUse::REGISTER, true);

This function already exists in ion/Lowering.h.

Comment 4

[Paul Wright]

Was this landed?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Paul Wright from comment #4)
> Was this landed?

No I forgot about this bug, will land it next week.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch v2

For the most part just like the previous patch, but asking for a second review since this adds useBoxFixed (for LSetDOMProperty/LApplyArgsGeneric). The second register passed to it is not used on x64 but this seemed cleaner than adding #ifdef's to lowering. The SetDOMProperty change should fix bug 790464.

(In reply to Sean Stangl from comment #3)
> Comment on attachment 647891 [details] [diff] [review]
> Patch
> 
> Review of attachment 647891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> We also need to assert that non-fixed GPRs are not used at the same time
> that a temporary is requested for a callsite. Otherwise this patch doesn't
> cause the obnoxious case to be detected and just adds more details for
> coders to remember.

Good idea, done.

>
> This has different behavior than the previous code if isCall() &&
> alloc.isSnapshotInput(). I'm not sure how that case can occur. If it can't,
> could an assertion be added?

The behavior is the same with/without the patch, snapshot inputs always stop at the output half.

> 
> We want to be able to write useRegisterOrConstantAtStart(power) instead of
> useFixed(...) here. This patch still doesn't let us, but it should at least
> assert if we tried to.

I verified this indeed asserts now.

> 
> This function already exists in ion/Lowering.h.

Good catch.

Comment 7

[Jan de Mooij [:jandem]]

Green on try (so far): https://tbpl.mozilla.org/?tree=Try&rev=5e7bf652ee8

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 661779 [details] [diff] [review]
Patch v2

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

Great catch with Bug 790464.

::: js/src/ion/Lowering.cpp
@@ +713,5 @@
>      JS_ASSERT(power->type() == MIRType_Int32 || power->type() == MIRType_Double);
>  
>      if (power->type() == MIRType_Int32) {
> +        // Note: useRegisterAtStart here is safe, the temp is a GP register so
> +        // it will never get the same register.

There's no logic that would avoid assigning a register that's already in use as a temporary?

@@ +719,5 @@
>                                 tempFixed(CallTempReg0));
>          return defineFixed(lir, ins, LAllocation(AnyRegister(ReturnFloatReg)));
>      }
>  
> +    LPowD *lir = new LPowD(useRegisterAtStart(input), useRegisterAtStart(power),

... because if there isn't, then useRegisterAtStart(power) here isn't safe, since power may be an integer that will overlap with CallTempReg0.

@@ +838,5 @@
>      }
>  
>      if (ins->specialization() == MIRType_Double) {
>          JS_ASSERT(ins->type() == MIRType_Double);
>          JS_ASSERT(ins->lhs()->type() == MIRType_Double);

Should also JS_ASSERT(ins->rhs()->type() == MIRType_Double).

Comment 9

[Sean Stangl [:sstangl]]

Oh, disregard the first two comments in Comment 8 -- PowD requires power to be a double.

Comment 10

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ee23a407dfe2

Pushed for Jan -- it looks like almost every crash is related to this bug. Things should look better in tomorrow's nightly.

Comment 11

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/ee23a407dfe2