1044212 - IonMonkey: Rewrite alias analysis with EmulateStateOf<MemoryView>

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

In our current implementation of Alias Analysis, we keep a vector of alias sets with potential clusters within the alias set where we distinguish between values which might alias on or the other.

The goal of this bug is just to do some cosmetic changes before the ground work which would be part of Bug 897606.  This bug focus on using EmulateStateOf<MemoryView> (Bug 1040738) for implementing our alias analysis without changing the result that it produces.

This implies multiple changes such as adding a new visit function to wrap the accept function of the MDefinition.  Moving the EmulateStateOf function to a new file which can be used by the current scalar replacement implementation and alias analysis.  And converting the Alias Analysis to use this new model.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Part 1 - Wrap the accept function in a MDefinition* visit function.

This patch add a MDefinitionVisitor::visit function such that we can factor common parts of
visitors.

This is a useful trick to reduce the amount of changes to Alias Analysis
when transitionning to EmulateStateOf.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Part 2 - Move EmulateStateOf<...> to MIRAlgorithms.h.

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8463377 [details] [diff] [review]
Part 1 - Wrap the accept function in a MDefinition* visit function.

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

::: js/src/jit/MOpcodes.h
@@ +239,5 @@
>  {
>    public:
> +    // Handle task which is common to every instruction, and/or dispatch to
> +    // specialized visit function.
> +    inline bool visit(MDefinition *def);

This is being defined outside the class because it needs the definition of MDefinition. Would it make sense to just move the whole MDefinitionVisitor class into MIR.h, right after MDefinition? Then it could define visit inside the class.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #3)
> Comment on attachment 8463377 [details] [diff] [review]
> Part 1 - Wrap the accept function in a MDefinition* visit function.
> 
> Review of attachment 8463377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MOpcodes.h
> @@ +239,5 @@
> >  {
> >    public:
> > +    // Handle task which is common to every instruction, and/or dispatch to
> > +    // specialized visit function.
> > +    inline bool visit(MDefinition *def);
> 
> This is being defined outside the class because it needs the definition of
> MDefinition. Would it make sense to just move the whole MDefinitionVisitor
> class into MIR.h, right after MDefinition? Then it could define visit inside
> the class.

I think this make sense, especially since we are moving MIR instructions in a separated file.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Initialize OSR block starting state. r=

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Part 4 - Add the current MBasicBlock as argument of setEntryBlockState.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Part 5 - Use EmulateStateOf for Alias Analysis implementation.

This patch use EmulateStateOf<...> to express what is currently done by the
alias analysis phase.

Bug 897606 would make this look more like the scalar replacement implementation.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8463471 [details] [diff] [review]
part 3 - Initialize OSR block starting state. r=

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

::: js/src/jit/MIRAlgorithms-inl.h
@@ +32,5 @@
> +            return false;
> +
> +        if (graph_.osrBlock() && !view.initStartingState(&states_[graph_.osrBlock()->id()]))
> +            return false;
> +    }

I'm not sure what the best place is for this, but could you add a comment somewhere in the interface mentioning this behavior? -- the behavior that startingBlock() returning null means that initStartingState is called on both the regular and OSR entries, and the RPO walk begins at the regular entry.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8463379 [details] [diff] [review]
Part 2 - Move EmulateStateOf<...> to MIRAlgorithms.h.

(Clearing reviews for now.)

Comment 10

[Nicolas B. Pierron [:nbp]]

Let's close this bug for the moment, as we changed the Alias Analysis without using the EmulateStateOf way of walking the graph.