Closed Bug 1038839 Opened 5 years ago Closed 5 years ago

Use type information for alias analysis

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jandem, Assigned: sstangl)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently for a loop like this:

for (var i=0; i<length; i++)
    dest[i] = src[0];

We assume dest[i] can alias src[0], so we can't hoist src[0]. But in practice dest and src will often have a different TypeObject, so if their type sets are disjoint we can hoist src[0] just fine. It should be pretty easy to implement MLoadElement::mightAlias and check for this case.

This is a stupid example of course, but it should be easy to prototype this and see how much it helps.

(I thought we had a bug for this, but I can't find it.)
I noticed this issue recently and found this bug, and just for fun thought I'd write up a quick patch.

In the example in Comment 0, this patches causes the MElements, MInitializedLength, and MLoadElement instructions of |src[0]| to be hoisted out of the loop.

This saves about 3% on that microbenchmark if the loop dominates, a laughably small amount. Obviously nothing else is even remotely noticeably affected, even though codegen is theoretically improved.
Attachment #8571617 - Flags: review?(jdemooij)
Comment on attachment 8571617 [details] [diff] [review]
0001-Implement-MLoadElement-mightAlias.patch

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

Sorry for the delay, looks great.

::: js/src/jit/MIR.cpp
@@ +4015,5 @@
> +    const MDefinition *storeObj = GetStoreObject(store);
> +    if (!storeObj)
> +        return true;
> +
> +    return input()->resultTypeSet()->objectsIntersect(storeObj->resultTypeSet());

Before this we should return true if storeObj->resultTypeSet() is nullptr.
Attachment #8571617 - Flags: review?(jdemooij) → review+
That's strange. I checked jit-tests before pushing... I'll investigate.
Attached patch Patch v2Splinter Review
The problem is that elements() doesn't necessarily return something of MIRType_Elements. It can also refer to the Object itself, when storage is inline, bypassing the elements altogether.

This new version passes all jit-tests with --ion, except for two which appear to time out on master as well.

If this patch is taken, I'd like to rename "elements" to "elementsOrObj" all over the place where relevant as a follow-up. Maybe "storage".
Attachment #8571617 - Attachment is obsolete: true
Flags: needinfo?(sstangl)
Attachment #8576929 - Flags: review?(jdemooij)
(In reply to Sean Stangl [:sstangl] from comment #6)
> The problem is that elements() doesn't necessarily return something of
> MIRType_Elements. It can also refer to the Object itself, when storage is
> inline, bypassing the elements altogether.

Sounds to me that it might be better to keep the MElement to save the displacement, and always keep the same graph independently of the object representation. Then we can try to fold this address later, in the LIRGenerator.
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Sounds to me that it might be better to keep the MElement to save the
> displacement, and always keep the same graph independently of the object
> representation. Then we can try to fold this address later, in the
> LIRGenerator.

That would definitely be nicer, yeah.
Attachment #8576929 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/5c0bd25d0a24
Assignee: nobody → sstangl
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This was missing some MOZ_OVERRIDE annotations, which caused -Winconsistent-missing-override build warnings in clang 3.6 & newer. (which breaks warnings-as-errors builds w/ that compiler version)

I landed a trivial followup to add this annotation, with the blanket r+ that ehsan granted me for fixes of this sort over on bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b19e6bcc058a
Depends on: 1148375
You need to log in before you can comment on or make changes to this bug.