812596 - BaselineCompiler: Add GETELEM/SETELEM ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: Fallback path

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1: Slow path

Updated after the merge today.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add dense array stub

I tried to add hasStub to ICEntry instead of FallbackStub, but that lead to some complicated dependencies. Also

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Also

Sorry ignore this ;)

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 683056 [details] [diff] [review]
Part 2: Add dense array stub

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

Looks good except for the register usage issue on ARM.

I think what we should do is take ICStubCompiler::availableGeneralRegs, and specialize it for each platform.  The regset returned by this method can be all registers that it's OK to clobber in the stubcode, and are usable for masm calls (e.g. ScratchReg is excluded).  On x86 and x64, this set can include BaselineTailCallReg, on ARM it won't.

Then, you should be able to pull out scratchReg from that register set.  What do you think?

Canceling review for now until we figure out the best approach for this.

::: js/src/ion/BaselineIC.cpp
@@ +369,5 @@
> +    Register obj = masm.extractObject(R0, ExtractTemp0);
> +    masm.branchTestObjShape(Assembler::NotEqual, obj, shape, &failure);
> +
> +    // Load obj->elements in scratchReg.
> +    Register scratchReg = BaselineTailCallReg;

This won't work on ARM, because on ARM, BaselineTailCallReg cannot be used as scratch, since it's |lr|, and we won't be able to return to mainline code if we clobber it.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add dense array stub (v2)

Now takes a scratch register from the register set.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3: SETELEM slow path

SETELEM depends on the patches in this bug so I will just post them here.. This leaves the RHS Value on the stack and loads the object and index values in R0 and R1. Generated code looks reasonable and what's nice about it is that this is very similar to GETELEM.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 4: SETELEM dense array stub

Comment 9

[Kannan Vijayan [:djvj]]

Comment on attachment 683508 [details] [diff] [review]
Part 2: Add dense array stub (v2)

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

Cool :)  I'm just going to add a type-monitor IC bug for this and make this bug dependent on it, just as a reminder.

Comment 10

[Kannan Vijayan [:djvj]]

Comment on attachment 683522 [details] [diff] [review]
Part 4: SETELEM dense array stub

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

::: js/src/ion/BaselineIC.cpp
@@ +485,5 @@
> +    masm.branchTestMagic(Assembler::Equal, element, &failure);
> +
> +    // It's safe to overwrite R0 now.
> +    masm.loadValue(Address(BaselineStackReg, ICStackValueOffset), R0);
> +    masm.storeValue(R0, element);

Do we need to worry about updating the TypeObject for the array with the type of the value being set?

If so, we may have to specialize the stubs on the incoming types for the RHS value.

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/cb05e88b551e
https://hg.mozilla.org/projects/ionmonkey/rev/32ab3af7f779
https://hg.mozilla.org/projects/ionmonkey/rev/1c1cd2a6d76c
https://hg.mozilla.org/projects/ionmonkey/rev/b8159d750821