Local optimisations

[OlivierBlanvillain]

This RP implements a subset of the optimisation done by linker that can be done with local knowledge. This is a first iteration and many things still need to be polished.

@DarkDimius your last 3 commits didn't completely solve the issue with testPickling, after cherry-picking them on top of the rebased branch still results in <notype> popping up in unpickling. To be investigated tomorrow 😄

[dottybot]

I like your commit messages.

[DarkDimius]

I've just played with the current state of this branch. After fixed by @OlivierBlanvillain some of very important optimizations are not trigerring.

For this input:

case class CC(a: Int, b: Object)
object Lists{
  def foo(x: Any): Int = {
    val (a, b) = x match {
      case CC(s @ 1, CC(t, _)) =>
        (s , 2)
      case _ => (42, 43)
    }
    a + b
  }
  def booleans(a: Object) = {
    val (b1, b2) = (a.isInstanceOf[CC], a.isInstanceOf[List[Int]])
    (b1, b2) match {
      case (true, true) => true
      case (false, false) => true
      case _ => false
    }
  }
}

Linker(with global opts disabled) generates this bytecode:

    public int foo(Object x) {
        CC cC;
        int n = 0;
        int n2 = 0;
        if (x instanceof CC && 1 == (cC = (CC)x)._1() && cC._2() instanceof CC) {
            n = 1;
            n2 = 2;
        } else {
            n = 42;
            n2 = 43;
        }
        return n + n2;
    }

    public boolean booleans(Object a) {
        boolean bl = a instanceof CC;
        boolean bl2 = a instanceof List;
        if (bl && bl2 || !bl && !bl2) {
            return true;
        }
        return false;
}

While dotty -optimise generates this:

public int foo(Object x) {
        if (!(x instanceof CC) || (CC)x == null) ** GOTO lbl-1000
        var3_2 = (CC)x;
        s = var3_2._1();
        var5_4 = var3_2._2();
        if (1 == s && var5_4 instanceof CC && (CC)var5_4 != null) {
            var6_5 = (CC)var5_4;
            var6_5._2();
            v0 = new Tuple2((Object)BoxesRunTime.boxToInteger((int)1), (Object)BoxesRunTime.boxToInteger((int)2));
        } else lbl-1000: // 2 sources:
        {
            v0 = new Tuple2((Object)BoxesRunTime.boxToInteger((int)42), (Object)BoxesRunTime.boxToInteger((int)43));
        }
        var2_6 = v0;
        a = BoxesRunTime.unboxToInt((Object)var2_6._1());
        b = BoxesRunTime.unboxToInt((Object)var2_6._2());
        return a + b;
    }

    public boolean booleans(Object a) {
        boolean bl;
        Tuple2 tuple2 = new Tuple2((Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof CC)), (Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof List)));
        boolean b1 = BoxesRunTime.unboxToBoolean((Object)tuple2._1());
        boolean b2 = BoxesRunTime.unboxToBoolean((Object)tuple2._2());
        Tuple2 tuple22 = new Tuple2((Object)BoxesRunTime.boxToBoolean((boolean)b1), (Object)BoxesRunTime.boxToBoolean((boolean)b2));
        new Some((Object)new Tuple2(tuple22._1(), tuple22._2())); //1
        Tuple2 tuple23 = new Tuple2(tuple22._1(), tuple22._2());
        boolean bl2 = BoxesRunTime.unboxToBoolean((Object)tuple23._1());
        boolean bl3 = BoxesRunTime.unboxToBoolean((Object)tuple23._2());
        if (bl2 && bl3) {
            bl = true;
        } else {
            new Some((Object)new Tuple2(tuple22._1(), tuple22._2())); // 1
            Tuple2 tuple24 = new Tuple2(tuple22._1(), tuple22._2());
            boolean bl4 = BoxesRunTime.unboxToBoolean((Object)tuple24._1());
            boolean bl5 = BoxesRunTime.unboxToBoolean((Object)tuple24._2());
            bl = bl4 ? false : !bl5;
        }
        return bl;
    }

It seems that at least two optimizations did not trigger:

1. elimination of unnececarry allocation of Some-s that are not used. (marked //1)
2. on-stack allocation of Tupple (this would also remove boxing).

In any case, getting this in is an improvement as without this dotty generates even worse code:

 public int foo(Object x) {
        var3_2 = x;
        if (!(var3_2 instanceof CC)) ** GOTO lbl-1000
        var4_3 = (CC)var3_2;
        if (CC$.MODULE$.unapply((CC)var3_2) == null) ** GOTO lbl-1000
        var5_4 = CC$.MODULE$.unapply((CC)var3_2);
        s = var5_4._1();
        var7_6 = var5_4._2();
        if (1 != s) ** GOTO lbl-1000
        var8_7 = s;
        if (!(var7_6 instanceof CC)) ** GOTO lbl-1000
        var9_8 = (CC)var7_6;
        if (CC$.MODULE$.unapply((CC)var7_6) != null) {
            var10_9 = CC$.MODULE$.unapply((CC)var7_6);
            var11_10 = var10_9._2();
            v0 = Tuple2..MODULE$.apply((Object)BoxesRunTime.boxToInteger((int)1), (Object)BoxesRunTime.boxToInteger((int)2));
        } else lbl-1000: // 5 sources:
        {
            v0 = Tuple2..MODULE$.apply((Object)BoxesRunTime.boxToInteger((int)42), (Object)BoxesRunTime.boxToInteger((int)43));
        }
        var2_11 = v0;
        a = BoxesRunTime.unboxToInt((Object)var2_11._1());
        b = BoxesRunTime.unboxToInt((Object)var2_11._2());
        return a + b;
    }

    /*
     * Enabled force condition propagation
     * Lifted jumps to return sites
     */
    public boolean booleans(Object a) {
        Tuple2 tuple2 = Tuple2..MODULE$.apply((Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof CC)), (Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof List)));
        boolean b1 = BoxesRunTime.unboxToBoolean((Object)tuple2._1());
        boolean b2 = BoxesRunTime.unboxToBoolean((Object)tuple2._2());
        Tuple2 tuple22 = Tuple2..MODULE$.apply((Object)BoxesRunTime.boxToBoolean((boolean)b1), (Object)BoxesRunTime.boxToBoolean((boolean)b2));
        Option option = Tuple2..MODULE$.unapply(tuple22);
        if (!option.isEmpty()) {
            Tuple2 tuple23 = (Tuple2)option.get();
            boolean bl = BoxesRunTime.unboxToBoolean((Object)tuple23._1());
            boolean bl2 = BoxesRunTime.unboxToBoolean((Object)tuple23._2());
            if (bl) {
                boolean bl3 = bl;
                if (bl2) {
                    boolean bl4 = bl2;
                    return true;
                }
            }
        }
        Option option2 = Tuple2..MODULE$.unapply(tuple22);
        if (option2.isEmpty()) return false;
        Tuple2 tuple24 = (Tuple2)option2.get();
        boolean bl = BoxesRunTime.unboxToBoolean((Object)tuple24._1());
        boolean bl5 = BoxesRunTime.unboxToBoolean((Object)tuple24._2());
        if (bl) return false;
        boolean bl6 = bl;
        if (bl5) return false;
        boolean bl7 = bl5;
        return true;
    }
}

[DarkDimius]

after discussion with @OlivierBlanvillain: Dotty currently has an important optimization disable(inlineLocalObjects) and this is why Dotty-generated bytecode is worse.

[OlivierBlanvillain]

This is ready for review

[DarkDimius]

For the record: a lot of discussion happened here: #2545

[DarkDimius]

note that commits are in wrong order due to https://help.github.com/articles/why-are-my-commits-in-the-wrong-order/

[DarkDimius]

this test is in the PR twice and it's broken. I'm working on a fix for it.

[DarkDimius]

I have fixed Simplify to perfrom correct transformation of all tests that were in tests/run-not-optimised.
Now ALL tests pass under -optimize.

[DarkDimius]

I started reviewing this PR. A lot of other optimizations didn't trigger.

I've enabled them and here's the code generated by dotty now:

 public int foo(Object x) {
        if (!(x instanceof CC) || (CC)x == null) ** GOTO lbl-1000
        var3_2 = (CC)x;
        var4_3 = var3_2._2();
        if (1 == var3_2._1() && var4_3 instanceof CC && (CC)var4_3 != null) {
            v0 = new Tuple2((Object)BoxesRunTime.boxToInteger((int)1), (Object)BoxesRunTime.boxToInteger((int)2));
        } else lbl-1000: // 2 sources:
        {
            v0 = new Tuple2((Object)BoxesRunTime.boxToInteger((int)42), (Object)BoxesRunTime.boxToInteger((int)43));
        }
        var2_4 = v0;
        return BoxesRunTime.unboxToInt((Object)var2_4._1()) + BoxesRunTime.unboxToInt((Object)var2_4._2());
    }

    public boolean booleans(Object a) {
        Tuple2 tuple2;
        Tuple2 tuple22 = new Tuple2((Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof CC)), (Object)BoxesRunTime.boxToBoolean((boolean)(a instanceof List)));
        Tuple2 tuple23 = new Tuple2(tuple22._1(), tuple22._2());
        Tuple2 tuple24 = new Tuple2(tuple23._1(), tuple23._2());
        boolean bl = BoxesRunTime.unboxToBoolean((Object)tuple24._1()) && BoxesRunTime.unboxToBoolean((Object)tuple24._2()) ? true : !BoxesRunTime.unboxToBoolean((Object)(tuple2 = new Tuple2(tuple23._1(), tuple23._2()))._1()) && !BoxesRunTime.unboxToBoolean((Object)tuple2._2());
        return bl;
    }

It is a least readable, unlike previous ones.

[OlivierBlanvillain]

Why do we need both this val and an explicitly tpd.cpy everywhere?

[DarkDimius]

We don't really, but I made the change just to be sure.

…

On 30 May 2017 09:51:33 Olivier Blanvillain ***@***.***> wrote: OlivierBlanvillain commented on this pull request. > @@ -34,6 +34,9 @@ class Simplify extends MiniPhaseTransform with > IdentityDenotTransformer { private var symmetricOperations: Set[Symbol] = null var optimize = false + + override val cpy = tpd.cpy Why do we need both this val and an explicitly `tpd.cpy` everywhere? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2513 (review)

[DarkDimius]

Something is strange with the CI.
It says that a pos test has failed with "0 error(s) and 0 warning(s)".

[smarter]

This is a known issue with Vulpix swallowing exceptions that I reported to @felixmulder. The workaround is to run it locally, the exception will be present in the log file.

[OlivierBlanvillain]

@DarkDimius Latest commit (b7149c9) got lost in a push --force... I don't know if you are aware of this option, but if you use push --force-with-lease instead git adds an additional check to prevent force pushing on top of locally unknown commits (which is probably what happened here?)

[DarkDimius]

@OlivierBlanvillain, I dropped it intentionally. Do we still need it after #2557 ?

[OlivierBlanvillain]

I don't know (not sure if the PR is rebased on top of 2557), but I think it's better like this anyway

[DarkDimius]

All tests passed!
// and we rebased in a wrong moment so we ran the entire nightly integration suite.