977126 - Inline SetTypedObjectOffset() in ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Niko Matsakis [:nmatsakis]]

The intrinsic SetTypedObjectOffset() is used in self-hosted code in tight loops and should be inlined. It corresponds to some pointer bumps, very simple.

Comment 1

[Niko Matsakis [:nmatsakis]]

The current C definition for SetTypedObjectOffset() reads the base pointer from the buffer. This is unnecessary and is not how the inlined code should work. We should apply this patch and then translate it:

diff --git a/js/src/builtin/TypedObject.cpp b/js/src/builtin/TypedObject.cpp
index 7898c12..23b1abb 100644
--- a/js/src/builtin/TypedObject.cpp
+++ b/js/src/builtin/TypedObject.cpp
@@ -2725,11 +2725,14 @@ js::SetTypedObjectOffset(ThreadSafeContext *, unsigned argc, Value *vp)
     JS_ASSERT(args[1].isInt32());
 
     TypedObject &typedObj = args[0].toObject().as<TypedObject>();
-    int32_t offset = args[1].toInt32();
+    int32_t newOffset = args[1].toInt32();
 
     JS_ASSERT(typedObj.typedMem() != nullptr); // must be attached already
 
-    typedObj.setPrivate(typedObj.owner().dataPointer() + offset);
+    int32_t oldOffset = typedObj.offset();
+    uint32_t *basePointer = typedObj.dataPointer() - oldOffset;
+    JS_ASSERT(basePointer == typedObj.owner().dataPointer());
+    typedObj.setPrivate(basePointer + offset);
     typedObj.setReservedSlot(JS_TYPEDOBJ_SLOT_BYTEOFFSET, Int32Value(offset));
     return true;
 }

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126.diff

Comment 3

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8393615 [details] [diff] [review]
Bug977126.diff

Cancelling review; I saw some test failures I want to track down first.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126.diff

Better version of the patch that actually *passes* tests.

Comment 5

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8393725 [details] [diff] [review]
Bug977126.diff

Argh. And again further testing is revealing problems. Can't win with this patch. :)

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126-Part1.diff

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126-Part2.diff

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126-Part3.diff

Comment 9

[Niko Matsakis [:nmatsakis]]

OK, this version seems to actually work, though I haven't tested on ARM.

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126-Part2.diff

That was an obsolete patch, sorry. Here is the updated one.

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Bug977126-Part2.diff

And...somehow I uploaded that stale patch AGAIN. Sorry.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8394247 [details] [diff] [review]
Bug977126-Part2.diff

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

Looks good, r=me with nit below addressed.

::: js/src/jit/x64/Assembler-x64.h
@@ +448,5 @@
>            default:
>              MOZ_ASSUME_UNREACHABLE("unexpected operand kind");
>          }
>      }
> +    void subq(const Register &src, const Address &dest) {

Most Assembler methods take an Operand instead of Address, can you do that here too? Just add a switch that only handles the case we're interested in and a default with MOZ_ASSUME_UNREACHABLE.

subPtr can then do Operand(dest), that also matches what the x86 version does.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8394141 [details] [diff] [review]
Bug977126-Part3.diff

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

Looks good to me.

::: js/src/builtin/TypedObject.h
@@ +567,5 @@
>      // by the JIT.
>      static size_t dataOffset();
>  
> +    // Offset of the byte offset slot.
> +    static size_t byteOffsetOffset();

In general I believe these are named like |offsetOfByteOffset|. I see a lot of other methods in TO already named |xOffset| instead of |offsetOfX|, maybe a mass rename if it's not too much of a bother.

::: js/src/jit/CodeGenerator.cpp
@@ +4051,5 @@
> +    // but to make that work out we need the option to subtract from a
> +    // memory location in place. X86 can do this but ARM cannot, and
> +    // masm doesn't seem to have the relevant helper methods defined
> +    // as of the time of this writing (and I am too lazy^H^H^H^Hwise
> +    // to do it myself).

Clean up comment to be less colorful. :)

::: js/src/jit/MCallOptimize.cpp
@@ +1639,5 @@
> +      case types::TemporaryTypeSet::ForAllResult::ALL_FALSE:
> +      case types::TemporaryTypeSet::ForAllResult::EMPTY:
> +      case types::TemporaryTypeSet::ForAllResult::MIXED:
> +        return InliningStatus_NotInlined;
> +        break;

Superfluous break

::: js/src/jit/MIR.h
@@ +5620,5 @@
> +      : MBinaryInstruction(object, offset)
> +    {
> +        JS_ASSERT(object->type() == MIRType_Object);
> +        JS_ASSERT(offset->type() == MIRType_Int32);
> +        setResultType(MIRType_Undefined);

This should be MIRType_None. In grepping around I also saw MAbortPar's result type being set as MIRType_UNdefined as well. Could you also change that to MIRType_None too?

Comment 14

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8394139 [details] [diff] [review]
Bug977126-Part1.diff

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +2445,5 @@
>  
>  void
> +MacroAssemblerARMCompat::subPtr(const Register &src, const Address &dest)
> +{
> +    loadPtr(dest, ScratchRegister);

If dest is going to be complex, (e.g. [r0, 5000], which will require multiple instructions), its value should probably be cached in the second scratch register.

Comment 15

[Niko Matsakis [:nmatsakis]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #14)
> If dest is going to be complex, (e.g. [r0, 5000], which will require
> multiple instructions), its value should probably be cached in the second
> scratch register.

I don't know what the notation [r0, 5000] means, but the Address will typically be "reg+offset". Can you point me at an example that makes use of this second scratch register to do such caching? I didn't see one when poking around just now.

Comment 16

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bab702b3df10
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a3df7451c35c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/88a167e0ca30
try:      https://tbpl.mozilla.org/?tree=Try&rev=c6cd4d90f6fd

I am canceling the needinfo request -- if this becomes a perf obstacle, we can optimize later.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc0eace320d for breaking b2g emulator builds, just like it did in your try push:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37177164&tree=Mozilla-Inbound

Comment 18

[Niko Matsakis [:nmatsakis]]

Indeed, my apologies! I overlooked that.

Comment 19

[Niko Matsakis [:nmatsakis]]

Try run with fix: https://tbpl.mozilla.org/?tree=Try&rev=527c8cb306ba

Some reds, but everything looks independent of this patch. I retriggered a few runs just to check.

Comment 20

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd335914852
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d87a13b71afc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2daa537b62e6

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/90f74b01a4a5 (along with bug 991234 from the same push) for Windows-debug mochitest-2 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=37235091&tree=Mozilla-Inbound

Comment 22

[Douglas Crosher [:dougc]]

(In reply to Niko Matsakis [:nmatsakis] from comment #15)
> (In reply to Marty Rosenberg [:mjrosenb] from comment #14)
> > If dest is going to be complex, (e.g. [r0, 5000], which will require
> > multiple instructions), its value should probably be cached in the second
> > scratch register.
> 
> I don't know what the notation [r0, 5000] means, but the Address will
> typically be "reg+offset". Can you point me at an example that makes use of
> this second scratch register to do such caching? I didn't see one when
> poking around just now.

The ARM instruction can only encode a small offset for the reg+offset addressing mode, so if the offset is large then it will need to be firstly loaded into a register.  Marty suggests using the secondScratchReg which defaults to lr.  See other uses in the source.  Marty has a patch pending to detect overlapping uses of the scratch register.

Comment 23

[Niko Matsakis [:nmatsakis]]

The offset will always be fixed and small, in the current use at least.

Comment 24

[Niko Matsakis [:nmatsakis]]

I did another try run and don't see any windows mochitest bustage:

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

Comment 25

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d7846fe42546
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba787cb6c35
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22432664801b

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d7846fe42546
https://hg.mozilla.org/mozilla-central/rev/8ba787cb6c35
https://hg.mozilla.org/mozilla-central/rev/22432664801b