843038 - IonMonkey: temper LICM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

During LICM we hoist operation from in the loop out of the loop. Now there is a small fault when we decide when an instruction is in the loop.

Currently we just make sure the ins->block()->id() >= header->id(). But that also includes all instructions coming after the loop. It doesn't really make sense to let LICM hoist instruction from after the loop to before the loop... That's the task of GVN.

I saw this happening when I adjusted raytrace to inline more. I created a short version of it:

function test(a, b) {

  for (var i=0; i<100; i++) {

    var c = 2.5;

    if (i == 40 && b) {
      return Math.pow(a,c); // This will get hoisted
    }

    if (i == 40 && !b) {
      return Math.pow(a+1,c); // This will get hoisted
    }
  }
}

for (var i=0; i<10000; i++) {
  test(2.5, i%2==0)
}

Both Math.pow's will get hoisted, although they are not actually part of the loop. This results in 2 times executing math.pow, while we only take 1 branch at a time! So we do more work than necessary...

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Make isInLoop function more accurate

Only unmark the blocks of a loop after finishing optimizing that loop. 
=> Now we can just look if the block is marked to know if the instruction is in the loop.

Comment 2

[Hannes Verschore [:h4writer]]

FYI this doesn't yield an improvement in any benchmark score I'm aware off. But when inlining the same subset of v8 on raytrace, we will keep the same score instead of being 5x slower due to an unbox that get's hoisted above the loop. (Note: this isn't the whole story why we are 5x slower, but that's one of the contributing factors)

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 716020 [details] [diff] [review]
Make isInLoop function more accurate

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

Nice find, r=me with comment addressed (just make sure Kraken, especially gaussian-blur, does not regress).

::: js/src/ion/LICM.cpp
@@ +46,5 @@
>  
>          if (!loop.optimize())
>              return false;
> +
> +        graph.unmarkBlocks();

You should also add a graph.unmarkBlocks() call before the "continue;".

Comment 4

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb2083111af

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fb2083111af

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> FYI this doesn't yield an improvement in any benchmark score I'm aware off.
> But when inlining the same subset of v8 on raytrace, we will keep the same
> score instead of being 5x slower due to an unbox that get's hoisted above
> the loop. (Note: this isn't the whole story why we are 5x slower, but that's
> one of the contributing factors)

I meant earleyboyer, off course (instead of raytrace)!