Skip to content
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

fix for SI-5672 #596

Merged
merged 1 commit into from May 22, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/compiler/scala/tools/nsc/backend/icode/Members.scala
Expand Up @@ -257,11 +257,23 @@ trait Members {
var succ = bb
do {
succ = nextBlock(succ);
bb.removeLastInstruction
succ.toList foreach { i => bb.emit(i, i.pos) }
code.removeBlock(succ)
val lastInstr = bb.lastInstruction
/* Ticket SI-5672
* Besides removing the control-flow instruction at the end of `bb` (usually a JUMP), we have to pop any values it pushes.
* Examples:
* `SWITCH` consisting of just the default case, or
* `CJUMP(targetBlock, targetBlock, _, _)` ie where success and failure targets coincide (this one consumes two stack values).
*/
val oldTKs = lastInstr.consumedTypes
assert(lastInstr.consumed == oldTKs.size, "Someone forgot to override consumedTypes() in " + lastInstr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion should probably go inside the constructor of Instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the right thing to do but we aren't there yet. I've created a ticket for that: https://issues.scala-lang.org/browse/SI-5819


bb.removeLastInstruction
for(tk <- oldTKs.reverse) { bb.emit(DROP(tk), lastInstr.pos) }
succ.toList foreach { i => bb.emit(i, i.pos) }
code.removeBlock(succ)
exh foreach { e => e.covered = e.covered - succ }

nextBlock -= bb
exh foreach { e => e.covered = e.covered - succ }
} while (nextBlock.isDefinedAt(succ))
bb.close
} else
Expand Down
8 changes: 8 additions & 0 deletions src/compiler/scala/tools/nsc/backend/icode/Opcodes.scala
Expand Up @@ -436,6 +436,8 @@ trait Opcodes { self: ICodes =>
override def consumed = 1
override def produced = 0

override val consumedTypes = List(INT)

def flatTagsCount: Int = { var acc = 0; var rest = tags; while(rest.nonEmpty) { acc += rest.head.length; rest = rest.tail }; acc } // a one-liner
}

Expand Down Expand Up @@ -470,6 +472,8 @@ trait Opcodes { self: ICodes =>

override def consumed = 2
override def produced = 0

override val consumedTypes = List(kind, kind)
}

/** This class represents a CZJUMP instruction
Expand All @@ -489,6 +493,8 @@ trait Opcodes { self: ICodes =>

override def consumed = 1
override def produced = 0

override val consumedTypes = List(kind)
}


Expand All @@ -499,6 +505,8 @@ trait Opcodes { self: ICodes =>
case class RETURN(kind: TypeKind) extends Instruction {
override def consumed = if (kind == UNIT) 0 else 1
override def produced = 0

// TODO override val consumedTypes = List(kind)
}

/** This class represents a THROW instruction
Expand Down
Expand Up @@ -1646,7 +1646,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
// TODO: if patterns allow switch but the type of the scrutinee doesn't, cast (type-test) the scrutinee to the corresponding switchable type and switch on the result
if (regularSwitchMaker.switchableTpe(scrutSym.tpe)) {
val caseDefsWithDefault = regularSwitchMaker(cases map {c => (scrutSym, c)}, pt)
if (caseDefsWithDefault.length <= 2) None // not worth emitting a switch... also, the optimizer has trouble digesting tiny switches, apparently, so let's be nice and not generate them
if (caseDefsWithDefault isEmpty) None // not worth emitting a switch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the optimizer now happy with one-branch switches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the changes above result in that one-branch-switch becoming straight-line code, a comment in SI-5672 shows the javap output for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

else {
// match on scrutSym -- converted to an int if necessary -- not on scrut directly (to avoid duplicating scrut)
val scrutToInt: Tree =
Expand Down
109 changes: 52 additions & 57 deletions test/files/run/inline-ex-handlers.check
@@ -1,27 +1,23 @@
172c172
< locals: value x$1, value x1, value x2, value x
< locals: value x$1, value x1
---
> locals: value x$1, value x1, value x2, value x, variable boxed1
> locals: value x$1, value x1, variable boxed1
174c174
< blocks: [1,2,3,5,6,7]
< blocks: [1,2,3,4]
---
> blocks: [1,3,5,6]
180,182d179
> blocks: [1,3,4]
186a187,188
> 92 STORE_LOCAL(variable boxed1)
> 92 LOAD_LOCAL(variable boxed1)
195,197d196
< 92 JUMP 2
<
< 2:
194,196d190
< 92 JUMP 7
<
< 7:
204a199,200
> 92 STORE_LOCAL(variable boxed1)
> 92 LOAD_LOCAL(variable boxed1)
395c391
385c384
< blocks: [1,2,3,4,5,8,11,13,14,16]
---
> blocks: [1,2,3,5,8,11,13,14,16,17]
419c415,424
409c408,417
< 103 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -34,15 +30,15 @@
> 106 LOAD_LOCAL(value x3)
> 106 IS_INSTANCE REF(class MyException)
> 106 CZJUMP (BOOL)NE ? 5 : 11
432,434d436
422,424d429
< 101 JUMP 4
<
< 4:
522c524
512c517
< blocks: [1,2,3,4,6,7,8,9,10]
---
> blocks: [1,2,3,4,6,7,8,9,10,11,12,13]
551c553,558
541c546,551
< 306 THROW(MyException)
---
> ? JUMP 11
Expand All @@ -51,7 +47,7 @@
> ? LOAD_LOCAL(variable monitor4)
> 305 MONITOR_EXIT
> ? JUMP 12
557c564,570
547c557,563
< ? THROW(Throwable)
---
> ? JUMP 12
Expand All @@ -61,7 +57,7 @@
> 304 MONITOR_EXIT
> ? STORE_LOCAL(value t)
> ? JUMP 13
563c576,589
553c569,582
< ? THROW(Throwable)
---
> ? STORE_LOCAL(value t)
Expand All @@ -78,30 +74,29 @@
> 310 CALL_PRIMITIVE(EndConcat)
> 310 CALL_METHOD scala.Predef.println (dynamic)
> 310 JUMP 2
587c613
577c606
< catch (Throwable) in ArrayBuffer(7, 8, 9, 10) starting at: 6
---
> catch (Throwable) in ArrayBuffer(7, 8, 9, 10, 11) starting at: 6
590c616
580c609
< catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10) starting at: 3
---
> catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10, 11, 12) starting at: 3
622c648
612c641
< blocks: [1,2,3,4,5,6,7,9,10]
---
> blocks: [1,2,3,4,5,6,7,9,10,11,12]
646c672,673
636c665,671
< 78 THROW(IllegalArgumentException)
---
> ? STORE_LOCAL(value e)
> ? JUMP 11
647a675,679
>
> 11:
> 81 LOAD_LOCAL(value e)
> ? STORE_LOCAL(variable exc1)
> ? JUMP 12
>
675c707,721
665c700,714
< 81 THROW(Exception)
---
> ? STORE_LOCAL(variable exc1)
Expand All @@ -119,15 +114,15 @@
> 84 STORE_LOCAL(variable result)
> 84 LOAD_LOCAL(variable exc1)
> 84 THROW(Throwable)
697c743
687c736
< catch (<none>) in ArrayBuffer(4, 6, 7, 9) starting at: 3
---
> catch (<none>) in ArrayBuffer(4, 6, 7, 9, 11) starting at: 3
723c769
713c762
< blocks: [1,2,3,4,5,6,9,12,14,17,18,19,22,25,27,28,30,31]
---
> blocks: [1,2,3,4,5,6,9,12,14,17,18,19,22,25,27,28,30,31,32,33,34]
747c793,800
737c786,793
< 172 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -138,12 +133,12 @@
> 170 STORE_LOCAL(value x3)
> 170 SCOPE_ENTER value x3
> 170 JUMP 18
803c856,857
793c849,850
< 177 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
> ? JUMP 33
807c861,868
797c854,861
< 170 THROW(Throwable)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -154,17 +149,17 @@
> 169 STORE_LOCAL(value x3)
> 169 SCOPE_ENTER value x3
> 169 JUMP 5
840c901,902
830c894,895
< 182 THROW(MyException)
---
> ? STORE_LOCAL(variable exc2)
> ? JUMP 34
844c906,907
834c899,900
< 169 THROW(Throwable)
---
> ? STORE_LOCAL(variable exc2)
> ? JUMP 34
845a909,921
835a902,914
> 34:
> 184 LOAD_MODULE object Predef
> 184 CONSTANT("finally")
Expand All @@ -178,19 +173,19 @@
> 185 LOAD_LOCAL(variable exc2)
> 185 THROW(Throwable)
>
866c942
856c935
< catch (Throwable) in ArrayBuffer(17, 18, 19, 22, 25, 27, 28, 30) starting at: 4
---
> catch (Throwable) in ArrayBuffer(17, 18, 19, 22, 25, 27, 28, 30, 32) starting at: 4
869c945
859c938
< catch (<none>) in ArrayBuffer(4, 5, 6, 9, 12, 17, 18, 19, 22, 25, 27, 28, 30) starting at: 3
---
> catch (<none>) in ArrayBuffer(4, 5, 6, 9, 12, 17, 18, 19, 22, 25, 27, 28, 30, 32, 33) starting at: 3
895c971
885c964
< blocks: [1,2,3,6,7,8,11,14,16,17,19]
---
> blocks: [1,2,3,6,7,8,11,14,16,17,19,20]
919c995,1002
909c988,995
< 124 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -201,15 +196,15 @@
> 122 STORE_LOCAL(value x3)
> 122 SCOPE_ENTER value x3
> 122 JUMP 7
979c1062
969c1055
< catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 14, 16, 17, 19) starting at: 3
---
> catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 14, 16, 17, 19, 20) starting at: 3
1005c1088
995c1081
< blocks: [1,2,3,4,5,8,11,15,16,17,19]
---
> blocks: [1,2,3,5,8,11,15,16,17,19,20]
1029c1112,1121
1019c1105,1114
< 148 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -222,15 +217,15 @@
> 154 LOAD_LOCAL(value x3)
> 154 IS_INSTANCE REF(class MyException)
> 154 CZJUMP (BOOL)NE ? 5 : 11
1050,1052d1141
1040,1042d1134
< 145 JUMP 4
<
< 4:
1285c1374
1275c1367
< blocks: [1,2,3,4,5,7]
---
> blocks: [1,2,3,4,5,7,8]
1309c1398,1405
1299c1391,1398
< 38 THROW(IllegalArgumentException)
---
> ? STORE_LOCAL(value e)
Expand All @@ -241,16 +236,16 @@
> 42 CONSTANT("IllegalArgumentException")
> 42 CALL_METHOD scala.Predef.println (dynamic)
> 42 JUMP 2
1358c1454
1348c1447
< blocks: [1,2,3,4,5,8,11,13,14,16,17,19]
---
> blocks: [1,2,3,5,8,11,13,14,16,17,19,20]
1382c1478,1479
1372c1471,1472
< 203 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
> ? JUMP 20
1402c1499,1508
1392c1492,1501
< 209 THROW(MyException)
---
> ? STORE_LOCAL(value ex5)
Expand All @@ -263,15 +258,15 @@
> 212 LOAD_LOCAL(value x3)
> 212 IS_INSTANCE REF(class MyException)
> 212 CZJUMP (BOOL)NE ? 5 : 11
1415,1417d1520
1405,1407d1513
< 200 JUMP 4
<
< 4:
1477c1580
1467c1573
< blocks: [1,2,3,4,5,7]
---
> blocks: [1,2,3,4,5,7,8]
1501c1604,1611
1491c1597,1604
< 58 THROW(IllegalArgumentException)
---
> ? STORE_LOCAL(value e)
Expand All @@ -282,11 +277,11 @@
> 62 CONSTANT("RuntimeException")
> 62 CALL_METHOD scala.Predef.println (dynamic)
> 62 JUMP 2
1550c1660
1540c1653
< blocks: [1,2,3,4]
---
> blocks: [1,2,3,4,5]
1570c1680,1685
1560c1673,1678
< 229 THROW(MyException)
---
> ? JUMP 5
Expand All @@ -295,19 +290,19 @@
> ? LOAD_LOCAL(variable monitor1)
> 228 MONITOR_EXIT
> 228 THROW(Throwable)
1576c1691
1566c1684
< ? THROW(Throwable)
---
> 228 THROW(Throwable)
1604c1719
1594c1712
< locals: value args, variable result, variable monitor2, variable monitorResult1
---
> locals: value exception$1, value args, variable result, variable monitor2, variable monitorResult1
1606c1721
1596c1714
< blocks: [1,2,3,4]
---
> blocks: [1,2,3,4,5]
1629c1744,1752
1619c1737,1745
< 245 THROW(MyException)
---
> ? STORE_LOCAL(value exception$1)
Expand All @@ -319,7 +314,7 @@
> ? LOAD_LOCAL(variable monitor2)
> 244 MONITOR_EXIT
> 244 THROW(Throwable)
1635c1758
1625c1751
< ? THROW(Throwable)
---
> 244 THROW(Throwable)
Expand Down