1246141 - Detect And/Or structure during IonBuilder

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Hannes Verschore [:h4writer]]

We have a dedicated pass that will restructure and/or structure into a better suitable and normal flow. Though that shouldn't be needed.

It is possible to do this during IonBuilder with no overhead. As a result a lot of complicated code can get removed. Since it is even easier to do it in IonBuilder.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: WIP

4 files changed, 83 insertions(+), 459 deletions(-)

Works in most cases. Though need to check for some fallout in jit-tests

Comment 2

[Nicolas B. Pierron [:nbp]]

Removing redundant Phi will break the block removal made in branch pruning, see the big huge comment at the top of the IonAnalysis file.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Removing redundant Phi will break the block removal made in branch pruning,
> see the big huge comment at the top of the IonAnalysis file.

Oops, reading too fast, this is removing the IsRedundantPhi with filter type.

Also, note that moving it sooner in the pipeline removes opportunities from GVN.

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Removing redundant Phi will break the block removal made in branch pruning,
> > see the big huge comment at the top of the IonAnalysis file.
> 
> Oops, reading too fast, this is removing the IsRedundantPhi with filter type.
> 
> Also, note that moving it sooner in the pipeline removes opportunities from
> GVN.

nbp, not sure what you are hinting at. This already happens during "Fold tests" currently and will now happen dynamically during IonBuilder. "Fold tests" is the first pass. As a result this will make no difference to GVN

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: bug1246141-fold-andor

This does the same as "Fold Tests". It detects and/or structures and patches the blocks to the right true/false block directly, instead of doing another test.
This approach is a little bit more advanced, since it also supports chains of and/or opcodes. ("Fold tests doesn't")

This all works and should be land ready. At first I was rather enthusiastic about this approach and it does what it should, without impacting timing on mainthread too much. Though I keep having more and more the feeling this is hiding/fixing the real issue at hand.

We made a nice system with passes that works off thread, except for the first step. This first step creates the CFG and with it also propagates types/resultTypeSet. Because we can only propagate the types during the first step, we are stuck to it and cannot improve it lateron. As a result "improveTypesAtCompare" and "foldAndOr" need to happen in the first step too, to be most effective. The more I'm working on it, the more that seems wrong! Both should be a offthread pass that has the same effectiveness. In order for that to work, we need Off Mainthread Mir Creation. Which failed the last time.

Therefore my request to feedback:
Do we want to land this? Given that this isn't the best approach? Knowing that the best approach is hard and something that might never work out? Or should we land this as an intermediate step, hoping OMMC will someday work?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8720712 [details] [diff] [review]
bug1246141-fold-andor

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

I just skimmed the patch, but I think this makes sense, considering it does the same as the current approach but removes > 360 lines of code.

One concern is that this runs on the main thread instead of the background thread, but the performance of this is negligible I think.

Don't we need this for Odin though?

::: js/src/jit/IonAnalysis.cpp
@@ -787,5 @@
> -    return true;
> -}
> -
> -bool
> -jit::FoldTests(MIRGraph& graph)

Nit: also remove FoldTests from the header file.

::: js/src/jit/IonBuilder.cpp
@@ +4257,5 @@
> +    MInstructionIterator end = block->end();
> +    for (MInstructionIterator it = block->begin(); it != end; it++) {
> +        MInstruction* ins = *it;
> +        if (ins->isPhi())
> +            continue;

Nit: MPhi is a definition but not an instruction, so I don't think it can show up here.

@@ +4458,5 @@
> +    if (!input->isPhi())
> +        return true;
> +
> +    if (input->block() != current) {
> +        if (input->block()->numSuccessors() != 0)

I think this should be 1 instead of 0

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Don't we need this for Odin though?

Good point...

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 9

[Jan de Mooij [:jandem]]

A lot has changed and we'd need this for both the Wasm-Ion frontend and WarpBuilder. I think we can close this.