New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specialized pattern matches don't eliminate impossible cases under virtpatmat #7172
Comments
Imported From: https://issues.scala-lang.org/browse/SI-7172?orig=1 |
@VladUreche said: But couldn't we just remove the ifs that cannot be true? This would yield a series of jumps from one label to the next, that could further be optimized down to the previous patmat output. Or is there a problem which I'm not seeing? |
@retronym said: But it doesn't seem like we currently eliminate impossible % scala210 -optimize -Xprint:jvm -e 'if (Int.box(1).isInstanceOf[Byte]) "byte" else "other"'
[[syntax trees at end of jvm]] // scalacmd9042292549797194950.scala
package <empty> {
object Main extends Object {
def main(argv: Array[String]): Unit = {
val args: Array[String] = argv;
{
{
new anonymous class anon$1();
()
}
}
};
def <init>(): Main.type = {
Main.super.<init>();
()
}
};
final class Main$$anon$1 extends Object {
def <init>(): anonymous class anon$1 = {
Main$$anon$1.super.<init>();
if (scala.Int.box(1).$isInstanceOf[Byte]())
"byte"
else
"other";
()
}
}
} I don't suppose that would be such a tough optimization to add. |
@VladUreche said: |
@VladUreche said: $ cat patmat.scala
object Test {
def foo(l: List[_]) = l match {
case Nil => "okay"
case _ :: _ => "okay"
case ll: List[_] => "dead code"
}
}
$ ../build/quick/bin/scalac patmat.scala -Xprint:cleanup
patmat.scala:5: warning: unreachable code
case ll: List[_] => "dead code"
^
[[syntax trees at end of cleanup]] // patmat.scala
package <empty> {
object Test extends Object {
def foo(l: List): String = {
case <synthetic> val x1: List = l;
...
case11(){
if (x1.$isInstanceOf[List]())
matchEnd8("dead code")
else
case12()
};
...
};
...
}
} |
@retronym said (edited on Feb 22, 2013 11:29:04 PM UTC): |
@VladUreche said: For a backend phase, I'm not sure it's worth tying to icode, as the new backend is already knocking at our door. WDYT? |
@retronym said: void SharkTopLevelBlock::do_instance_check() {
// Get the class we're checking against
bool will_link;
ciKlass *check_klass = iter()->get_klass(will_link);
// Get the class of the object we're checking
ciKlass *object_klass = xstack(0)->type()->as_klass();
// Can we optimize this check away?
if (static_subtype_check(check_klass, object_klass)) {
if (bc() == Bytecodes::_instanceof) {
pop();
push(SharkValue::jint_constant(1));
}
return;
}
// Need to check this one at runtime
if (will_link)
do_full_instance_check(check_klass);
else
do_trapping_instance_check(check_klass);
} But it would be nice to eliminate them on our side on a bytecode size argument. I'm not sure whether this (@specialized + type casing) comes up enough to make it high priority. But we could immediately remove the code from Duplicators that no longer has the intended effect. |
@VladUreche said: Btw, shark is the llvm backend of the vm, but I couldn't quickly identify the same for c1 and opto. They probably have it, it's just that I'm not familiar enough with the source code to quickly locate it. |
@retronym said: It's already doing something similar for refinement types, although it doesn't deal with scala> (null: A with B).isInstanceOf[A with B]
res6: Boolean = true // should be false |
@retronym said: retronym/scala@scala:2.10.x...retronym:ticket/7172 With this in place, the original code is now compiled to: <specialized> class Foo$mcS$sp extends Foo {
override <specialized> def test(x: Short): Unit = Foo$mcS$sp.this.test$mcS$sp(x);
override <specialized> def test$mcS$sp(x: Short): Unit = scala.this.Predef.println({
case <synthetic> val x1: Short = x;
case12(){
if (if (scala.Short.box(x1).==(null))
false
else
false)
matchEnd11("bool")
else
case13()
};
case13(){
if (if (scala.Short.box(x1).==(null))
false
else
false)
matchEnd11("byte")
else
case14()
};
case14(){
if (if (scala.Short.box(x1).==(null))
false
else
true)
matchEnd11("short")
else
case15()
};
// ... Code:
Stack=2, Locals=3, Args_size=2
0: getstatic #21; //Field scala/Predef$.MODULE$:Lscala/Predef$;
3: iload_1
4: invokestatic #27; //Method scala/runtime/BoxesRunTime.boxToShort:(S)Ljava/lang/Short;
7: ifnonnull 14
10: iconst_0
11: goto 15
14: iconst_0
15: ifeq 24
18: ldc #29; //String bool
20: astore_2
21: goto 174
24: iload_1
25: invokestatic #27; //Method scala/runtime/BoxesRunTime.boxToShort:(S)Ljava/lang/Short;
28: ifnonnull 35
31: iconst_0
32: goto 36
35: iconst_0
36: ifeq 45
39: ldc #31; //String byte
41: astore_2
42: goto 174
45: iload_1
46: invokestatic #27; //Method scala/runtime/BoxesRunTime.boxToShort:(S)Ljava/lang/Short;
49: ifnonnull 56
52: iconst_0
53: goto 57
56: iconst_1
57: ifeq 66
60: ldc #33; //String short
62: astore_2
63: goto 174
66: iload_1
... So we still need to teach the optimizer that |
@dragos said: AFAIK the optimizer doesn't do any optimizations around conditional jumps, so you'd need to code that from scratch. It'd be worth it (and could also handle |
@retronym said: We could certainly warn about them in the type checker. I think the idea has come up before, but the usual answer is that if you want the warnings, you should use a pattern match, rather than an Another approach, albeit less generally useful, would be to avoid emitting the |
Considering
test/files/specialized/spec-patmatch.scala
/ scala/scala@b94c6e0-Xoldpatmat
With virtpatmat (in which
Match
nodes are eliminated before the specialize phase)I'm going to ask Iulian what the motivation was for getting
Duplicators
to strip out the impossible cases; was it performance, avoidance of errors from the subsequent match translation, or both?The simplest resolution here will be to remove the now-redundant code from
Duplicators
and live with the extra bytecode.The text was updated successfully, but these errors were encountered: