814715 - ParallelArray: Questionable codegen in CodeGenerator::visitParNew(LParNew *lir)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

ParallelArray code: Questionable codegen in CodeGenerator::visitParNew(LParNew *lir)

Reference:

https://github.com/syg/iontrail/blob/de6b15bcea20b9dd95b5c68e4965da948ef63ce7/js/src/ion/CodeGenerator.cpp#L1908

In the code for CodeGenerator::visitParNew, there is questionable code (copied below, linked above):

    Register tempReg1 = ToRegister(lir->getTemp0());
    ...

    // tempReg1 = (ArenaLists*) forkJoinSlice->arenaLists
    masm.loadPtr(Address(threadContextReg, offsetof(ForkJoinSlice, arenaLists)), tempReg1);

    // tempReg1 = (FreeSpan*) &objReg.freeLists[thingKind]
    uintptr_t freeSpanOffset = gc::ArenaLists::getFreeListOffset(allocKind);
    masm.addPtr(Imm32(freeSpanOffset), tempReg1);

That's the last reference of 'tempReg1' in the code; so something is almost certainly wrong.  We either have dead code building up this value and then not using it, or the code gen that follows this is missing a use of 'tempReg1'.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

see also conversation logged at:
  http://logbot.glob.com.au/?c=mozilla%23pjs&s=23+Nov+2012&e=23+Nov+2012
where niko notes:

15:56	nmatsakis	I think what it SHOULD Be
15:56	nmatsakis	is that the lines which load tempReg2 and tempReg3
15:56	nmatsakis	should be:
15:56	nmatsakis	masm.loadPtr(Address(**tempReg1**, offsetof(gc::FreeSpan, first)), tempReg2);
15:56	nmatsakis	(note the **)
15:56	nmatsakis	and similar for tempReg3

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: use tempReg1.

I've run jit_test.py with this patch (though admittedly on a repo that also had other experimental changes applied; so not as pure a testbed as i would like).  

No alarm bells were set off.

Comment 3

[Niko Matsakis [:nmatsakis]]

Comment on attachment 684720 [details] [diff] [review]
patch A v1: use tempReg1.

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

Looks perfect.  It'd be nice if we could make a test that exposes this bug first so we can validate it is fixed.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Pulled into iontrail:

  https://github.com/syg/iontrail/commit/94bb1f9441b2173234039d6e4dc632acc83b87b4