1189117 - IonMonkey: ScalarReplacement introduces aliases which prevent hoisting by LICM

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Spenser Andrew Bauman [:sbauman]]

Attachment: example.js

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150728132641

Steps to reproduce:

The ScalarReplacement pass introduces MObjectState instructions which alias analysis determines to alias otherwise loop invariant. In the case of the example attached, the operations affected are MElements, MArrayLength, and MInitializedLength operations acting on objects unrelated to objects affected by ScalarReplacement. This benchmark ends up faster by disabling scalar replacement.

Comment 1

[Spenser Andrew Bauman [:sbauman]]

Attachment: object-state-alias.patch

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8640803 [details] [diff] [review]
object-state-alias.patch

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

::: js/src/jit/AliasAnalysis.cpp
@@ +206,5 @@
>                  continue;
>  
> +            // For the purposes of AliasAnalysis, we treat ObjectState
> +            // operations introduced by ScalarAnalysis as effect free.
> +            if (def->isObjectState())

This is true for all instructions which are recovered on bailout, as all memory manipulations represented by these instructions cannot alias any others.  Thus, these are not "side"-effects, and thus can be safely ignored by the Alias Analysis phase.

Scalar Replacement adds MObjectState / MArrayState if everything is known about the object, which means that we know that no other alias set will alias the object which is being replaced.

Comment 3

[Jan de Mooij [:jandem]]

Can't we just give these instructions an empty alias set then, instead of special-casing them in AliasAnalysis.cpp? What am I missing?

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Can't we just give these instructions an empty alias set then, instead of
> special-casing them in AliasAnalysis.cpp? What am I missing?

Ideally yes.

One of the ""reasons"" we cannot do that is because of MResumePoint::addStore which asserts that the store instruction (MObjectState / MArrayState) is effectful, it might have been removed by DCE.

I do not think this assertion still holds.

Comment 5

[Spenser Andrew Bauman [:sbauman]]

Attachment: object-state-alias.patch

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

Spenser: Have you tried what Jan suggested in comment 3, i-e to return a None aliasSet on the MObjectState / MArrayState?

Comment 7

[Spenser Andrew Bauman [:sbauman]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Spenser: Have you tried what Jan suggested in comment 3, i-e to return a
> None aliasSet on the MObjectState / MArrayState?

That was, in fact, the first thing I attempted. Doing so ran afoul of several |isEffectful| assertions. I also had some concerns that this might allow the LICM pass to hoist the MObjectState and MArrayState instructions. Perhaps if we added another AliasSet type which encodes "effectful but aliases nothing".

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Jan de Mooij [:jandem] from comment #3)
> > Can't we just give these instructions an empty alias set then, instead of
> > special-casing them in AliasAnalysis.cpp? What am I missing?
> 
> Ideally yes.
> 
> One of the ""reasons"" we cannot do that is because of
> MResumePoint::addStore which asserts that the store instruction
> (MObjectState / MArrayState) is effectful, it might have been removed by DCE.
> 
> I do not think this assertion still holds.

Correction, this assertion is still needed because addStore does not add any uses, as we expected to have a lot of uses of these states.  As the addStore function does not add any uses, the state instructions are valid candidates for the removal by DCE.

So for the moment, I think this patch is valid, but sad.

Comment 9

[Spenser Andrew Bauman [:sbauman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc95150a636d

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c933c71431c4

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c933c71431c4

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > (In reply to Jan de Mooij [:jandem] from comment #3)
> > > Can't we just give these instructions an empty alias set then, instead of
> > > special-casing them in AliasAnalysis.cpp? What am I missing?
> > 
> > Ideally yes.
> > 
> > One of the ""reasons"" we cannot do that is because of
> > MResumePoint::addStore which asserts that the store instruction
> > (MObjectState / MArrayState) is effectful, it might have been removed by DCE.
> > 
> > I do not think this assertion still holds.
> 
> Correction, this assertion is still needed because addStore does not add any
> uses, as we expected to have a lot of uses of these states.  As the addStore
> function does not add any uses, the state instructions are valid candidates
> for the removal by DCE.
> 
> So for the moment, I think this patch is valid, but sad.

And what about making those use the Guard flag?

Comment 13

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> (In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > Correction, this assertion is still needed because addStore does not add any
> > uses, as we expected to have a lot of uses of these states.  As the addStore
> > function does not add any uses, the state instructions are valid candidates
> > for the removal by DCE.
> 
> And what about making those use the Guard flag?

"Guard" is not the right semantic here.  Ideally we might want to remove these instructions if no other resume points use them, unfortunately, this would imply to keep track of the uses.

So effectively, this is identical to making these instruction guards.

Another solution would be to keep track of the uses, and start converging towards a sea of nodes instead of having adhoc solutions.