Closed Bug 779411 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: sstangl, Assigned: jandem)

References

Details

(Whiteboard: [ion:p2])

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Use at-start explicitly rather than implicitly for call instructions, passes jit-tests on 32-bit and 64-bit.
Attachment #647891 - Flags: review?(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.
Attachment #647891 - Flags: review?(sstangl) → review+
Was this landed?
(In reply to Paul Wright from comment #4)
> Was this landed?

No I forgot about this bug, will land it next week.
Blocks: 790464
Attached patch Patch v2Splinter Review
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.
Attachment #647891 - Attachment is obsolete: true
Attachment #661779 - Flags: review?(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).
Attachment #661779 - Flags: review?(sstangl) → review+
Oh, disregard the first two comments in Comment 8 -- PowD requires power to be a double.
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.
https://hg.mozilla.org/mozilla-central/rev/ee23a407dfe2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Duplicate of this bug: 790464
Duplicate of this bug: 791657
Duplicate of this bug: 790464
No longer blocks: 790464
You need to log in before you can comment on or make changes to this bug.