new bytecode emitter, GenBCode (11th attempt) #2620

Merged
merged 8 commits into from Jul 1, 2013

Conversation

Projects
None yet
10 participants
Contributor

magarciaEPFL commented Jun 2, 2013

This PR re-submission supersedes #2603 and takes into account review comments so far.

Compared to the previous PR, pipelining is left for a future PR (same goes for "built-in" dead-code elimination and jumps-chain collapsing). This PR includes just the bytecode-emitter.

Let's see what dbuild thinks about GenBCode (after changing in ScalaSettings the default for the neo option from GenASM as in this PR to GenBCode).

review by @gkossakowski or @JamesIry or @retronym or @paulp

magarciaEPFL added some commits May 11, 2013

@magarciaEPFL magarciaEPFL an ICode InvokeStyle can now answer whether it isSuper c4d1217
@magarciaEPFL magarciaEPFL new bytecode emitter, GenBCode, for now under a flag
GenBCode is a drop-in replacement for GenASM with several advantages:

  - faster: ICode isn't necessary anymore.
    Instead, the ASTs delivered by CleanUp (an expression language)
    are translated directly into a stack-language (ASM Tree nodes)

  - future-proofing for Java 8 (MethodHandles, invokedynamic).

  - documentation included, shared mutable state kept to a minimum,
    all contributing to making GenBCode more maintainable
    than its counterpart (its counterpart being GenICode + GenASM).

A few tests are modified in this commit, for reasons given below.

(1) files/neg/case-collision

    Just like GenASM, GenBCode also detects output classfiles
    differing only in case. However the error message differs
    from that of GenASM (collisions may be show in different order).
    Thus the original test now has a flags file containing -neo:GenASM
    and a new test (files/neg/case-collision2) has been added
    for GenBCode. The .check files in each case show expected output.

(2) files/pos/t5031_3

    Currently the test above doesn't work with GenBCode
    (try with -neo:GenBCode in the flags file)
    The root cause lies in the fix to
    https://issues.scala-lang.org/browse/SI-5031
    which weakened an assertion in GenASM
    (GenBCode keeps the original assertion).
    Actually that ticket mentions the fix is a "workaround"

(3) files/run/t7008-scala-defined

    This test also passes only under GenASM and not GenBCode,
    thus the flags file. GenASM turns a bling eye to:

      An AbstractTypeSymbol (SI-7122) has reached the bytecode emitter,
      for which no JVM-level internal name can be found:
      ScalaClassWithCheckedExceptions_1.E1

    The error message above (shown by GenBCode) highlights
    there's no ScalaClassWithCheckedExceptions_1.E1 class,
    thus shouldn't show up in the emitted bytecode
    (GenASM emits bytecode that mentions the inexistent class).
22ee2df
@magarciaEPFL magarciaEPFL two bytecode tests atune with bytecode by GenASM 5213d23
Contributor

magarciaEPFL commented Jun 2, 2013

Good try, dbuild !!!

[info] [warn]   ::::::::::::::::::::::::::::::::::::::::::::::
[info] [warn]   ::          UNRESOLVED DEPENDENCIES         ::
[info] [warn]   ::::::::::::::::::::::::::::::::::::::::::::::
[info] [warn]   :: com.typesafe.sbt#sbt-ghpages;0.5.0: not found
[info] [warn]   :: com.typesafe#sbt-mima-plugin;0.1.3: not found
[info] [warn]   ::::::::::::::::::::::::::::::::::::::::::::::
[info] [warn] 
[info] [warn]   Note: Some unresolved dependencies have extra attributes.  Check that these dependencies exist with the requested attributes.
[info] [warn]       com.typesafe.sbt:sbt-ghpages:0.5.0 (sbtVersion=0.12, scalaVersion=2.9.2)
[info] [warn]       com.typesafe:sbt-mima-plugin:0.1.3 (sbtVersion=0.12, scalaVersion=2.9.2)
[info] [warn] 
[info] sbt.ResolveException: unresolved dependency: com.typesafe.sbt#sbt-ghpages;0.5.0: not found
[info] unresolved dependency: com.typesafe#sbt-mima-plugin;0.1.3: not found
[info]  at sbt.IvyActions$.sbt$IvyActions$$resolve(IvyActions.scala:214)
[info]  at sbt.IvyActions$$anonfun$update$1.apply(IvyActions.scala:122)
Owner

adriaanm commented Jun 3, 2013

@cunei started a new build (thanks!), which seems to have been successful!

Member

gkossakowski commented Jun 3, 2013

@cunei: could we have a dbuild job that not only builds akka but also executes its tests?

Owner

adriaanm commented Jun 3, 2013

I actually created that job, so we should be able to do so. Trying to build all of akka.

Contributor

cunei commented Jun 3, 2013

Technically, dbuild will test all the sbt-based project, unless an explicit "run-tests: false" is added. In this case, it will try to run "test" of the only Akka subproject that was specified ("akka-actor"), which may or may not do anything at this time. Compiling (and testing) the entire Akka is another matter, however: Akka depends on a number of other libraries, including stuff like "zeromq-scala-binding" and "scalabuff". I am currently trying to track down and hammer into shape all of the necessary pieces; the full dbuild task is at https://jenkins-dbuild.typesafe.com:8499/job/Community1. I am still working on it, though.

Contributor

magarciaEPFL commented Jun 3, 2013

For testing in real-world conditions, the real-world GenBCode should be used instead (ie this PR + dead-code elimination + jump chains collapsing + removal of dangling exception handlers, as described at https://github.com/magarciaEPFL/scala/commit/b0b012e6636263ff982ee4f375ccce7419ed6e25 ).

That functionality was originally left out to simplify reviewing.

Member

gkossakowski commented Jun 3, 2013

@magarciaEPFL: the functionality you mentioned are all optimizations so whether tests pass shouldn't be affected by lack of those features, right?

Contributor

magarciaEPFL commented Jun 3, 2013

Dead-code elimination is something ICode + GenASM do nowadays by default, same goes for jump-chains collapsing (it's called "removal of JUMP-only blocks" there). If I remember correctly @JamesIry added dce by default because otherwise a VerifyError occurred (admittedly a corner case). Here's some background, quoting from https://github.com/magarciaEPFL/scala/commit/b0b012e6636263ff982ee4f375ccce7419ed6e25

Before writing classfiles with "optimization level zero"
(ie -neo:GenBCode) the very least we want to do is remove
dead code beforehand, so as to prevent an artifact of
stack-frames computation from showing up, the artifact described at
http://asm.ow2.org/doc/developer-guide.html#deadcode

That artifact results from the requirement by the Java 6 split verifier
that a stack map frame be available for each basic block,
even unreachable ones.

Contributor

magarciaEPFL commented Jun 3, 2013

One of the tickets showing why DCE is necessary: https://issues.scala-lang.org/browse/SI-7006

Member

gkossakowski commented Jun 3, 2013

Thanks for the pointers. I wasn't aware of that issue.

Owner

adriaanm commented Jun 3, 2013

SI-7006 does not affect correctness, right? "Just" worse performance.

Contributor

magarciaEPFL commented Jun 3, 2013

Without DCE by GenBCode then we must trust the code-patching approach of http://asm.ow2.org/doc/developer-guide.html#deadcode to emit verifiable code. By doing that, we may well end up debugging that code-patching approach rather than GenBCode.

For running tests, we can't compare apples (GenASM with built in DCE + removal of jump-only basic blocks) and oranges (GenBCode lobotomized not to perform DCE, nor jump-chains collapsing).

Owner

adriaanm commented Jun 3, 2013

Is there a concrete reason for not trusting ASM? (E.g., evidence of bugs in this area?)

We're not going to reject the PR that introduces the non-optimizing core of the new back-end because it generates slower code than the status quo. The simpler the PR, the likelier its success. Also, correctness and (evidence of) testing.

Follow-up PRs can introduce optimizations.

Contributor

magarciaEPFL commented Jun 3, 2013

Is there a concrete reason for not trusting ASM? (E.g., evidence of bugs in this area?)

The code-patching approach in question is ASM's last-ditch effort to deal with unfinished job by the compiler. I can't recall an example where that code-patching approach fails. I haven't tested it either. The testing I've done is of GenBCode with DCE in place. I can't predict how GenBCode will work (say, compiling akka) without those features.

Contributor

magarciaEPFL commented Jun 4, 2013

Just to clarify the status of this PR:

Done:
(a.1) all review comments for previous submissions have been addressed
(a.2) bootstrapping of scalac successful using GenBCode
(a.3) dbuild of akka-actors successful using GenBCode

Pending:
(b.1) configure dbuild to build and test all-of-akka (first with GenASM, then with GenBCode)
(b.2) ongoing discussion on whether the code-patching approach of ASM can be used instead of proper dead-code elimination

Any items you might want to add / change?

Owner

adriaanm commented Jun 4, 2013

As far as I can see, the net impact of this PR on our test suite is 4 tests that now only run under "-neo:GenASM", so I guess they don't work under the new back-end. No new regressions tests specific to the new back-end are introduced.

I agree being able to bootstrap and compile akka-actors inspires confidence. As discusses, I'd like to see some evidence of investigation regarding the differences between the new back-end and the old, preferably as automated tests. One test I can imagine is to compile the same code under the old and the new back-end and compare their output.

We have infrastructure for that: https://github.com/scala/scala/blob/master/src/partest/scala/tools/partest/BytecodeTest.scala

And people are using it: 386a5bd, 69109c0, b50a0d8, 3f0224c, c8fbba0, 5f3cd86, e9f6511, d8ba6af, 13caa49, b2117cf, b47bb0f, 494ba94, 71ea3e8

Owner

retronym commented Jun 4, 2013

@magarciaEPFL One area of the current backend that isn't particularly well tested is positions. Here's an example of one I fixed: #1754; it included a bug in the ICode machinery.

Do you consider this sort of bug to be high/medium/low risk under GenBCode? Are there any aspects of the new design that lend themselves to getting positions right? Should we invest in old-vs-new backend testing of this?

Contributor

magarciaEPFL commented Jun 5, 2013

@retronym

The fix to SI-6288 mentions the root cause of the problem:

ICode generation was assigning the position of the last label jump to all jumps to that particular label def.

With GenBCode, whatever line number information appears in bytecode is the result of explicit invocations to lineNumber(tree: Tree) https://github.com/magarciaEPFL/scala/commit/22ee2df1a58fbdfe2fa8344e4b6658745bc60d17#L8R463 which allows fine tuning that aspect.

Once we're dealing with ICode, all we can diagnose (as in the example) is that some JUMPs and their targets don't have the right positions. At the level of GenBCode we still have at our disposal the original Trees, whose structure still preserves lexical scoping, thus allowing more precise line numbers to be emitted.

Thanks to @dragos for the expert guidance as we wended our way through GenICode to the troublesome code. Chalk up another bug for mutability.

That would have been definitely less time consuming with GenBCode!

Contributor

magarciaEPFL commented Jun 5, 2013

@adriaanm

As far as I can see, the net impact of this PR on our test suite is 4 tests that now only run under "-neo:GenASM", so I guess they don't work under the new back-end.

You can also see that (quoting from the commit message):

  • files/neg/case-collision : Both GenASM and GenBCode emit an error message, but the wording differs (collisions may be shown in different order).
  • files/pos/t5031_3 : That's accepted by GenASM after an assertion was weakened. Are you saying the weaker assertion is right? More context on this in a separate comment.
  • files/run/t7008-scala-defined : A bug in the compiler is ignored by GenASM. The GenASM behavior is wrong: https://issues.scala-lang.org/browse/SI-7122 Same goes for https://issues.scala-lang.org/browse/SI-7552
  • test/files/jvm/bytecode-test-example and test/files/jvm/nooptimise/ The BytecodeTest fails on GenBCode output not because the GenBCode output is wrong, but because the test is only testing whether a certain code pattern appears in the output.

@adriaanm , as explained above, GenBCode is doing the right thing in all cases.

Contributor

magarciaEPFL commented Jun 5, 2013

@adriaanm , one of the tests you mentioned that you'd like GenBCode to pass is the following (quoting from the commit message). Are you saying the current behavior by GenASM is right? Could you elaborate, please?

files/pos/t5031_3

Currently the test above doesn't work with GenBCode
(try with -neo:GenBCode in the flags file)
The root cause lies in the fix to
https://issues.scala-lang.org/browse/SI-5031
which weakened an assertion in GenASM
(GenBCode keeps the original assertion).
Actually that ticket mentions the fix is a "workaround"
Member

gkossakowski commented Jun 5, 2013

@magarciaEPFL: the bytecode-test-example counts the number of null checks peformed. In the source code I see exactly 2 and that's what the test expects. How many checks the GenBCode emits?

Owner

adriaanm commented Jun 5, 2013

My point was that you're not adding new tests (while we'd agreed there would be do some kind of testing), but rather changing/disabling old ones. That tends to be an opportunity to learn something about the new or the old code. Maybe the old was to blame, maybe the new implementation. Maybe both.

Regarding pos/t5031_3, it seems this should compile. What's the difference under GenBCode?

Contributor

magarciaEPFL commented Jun 5, 2013

@gkossakowski

Oh well, it's all my fault, I was using a better optimizer too soon, before it's been merged into trunk.

This is the source program, aka test\files\jvm\bytecode-test-example\Foo_1.scala

class Foo_1 {
  def foo(x: AnyRef): Int = {
    val bool = x == null
    if (x != null)
      1
    else
      0
  }
}

With GenASM and -optimise we get the two conditional jumps that bytecode-test-example.check expects:

  public foo(Ljava/lang/Object;)I
   L0
    ALOAD 1
    IFNONNULL L1
   L1
    ALOAD 1
    IFNONNULL L2
   L3
    ICONST_0
    GOTO L4
   L2
    ICONST_1
   L4
    IRETURN
   L5
    LOCALVARIABLE this LFoo_1; L0 L5 0
    LOCALVARIABLE x Ljava/lang/Object; L0 L5 1
    MAXSTACK = 1
    MAXLOCALS = 2

BTW, the textual representation above was obtained with -Ygen-asmp <output-folder>

With the new optimizer (just intra-method optimizations suffice) two useful things happen:

  • dead-store elimination of the bool local
  • control flow that mirrors the lexical structure

In other words, there's one less conditional jump than what bytecode-test-example considers "the right answer"

  public foo(Ljava/lang/Object;)I
   L0
    ALOAD 1
    IFNULL L1
   L2
    ICONST_1
    IRETURN
   L1
    ICONST_0
    IRETURN
   L3
    LOCALVARIABLE this LFoo_1; L0 L3 0
    LOCALVARIABLE x Ljava/lang/Object; L0 L3 1
    MAXSTACK = 1
    MAXLOCALS = 2

BTW, the idea about "control flow that mirrors lexical structure" is not mine, I just read a paper about it and thought it was cool:

The Program Structure Tree: Computing Control Regions in Linear Time (1994)
by Richard Johnson , David Pearson , Keshav Pingali
http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.38.4257

Member

gkossakowski commented Jun 5, 2013

@magarciaEPFL: Yeah, I was guessing that you were running DCE and eliminating that one unnecessary null check. The test is just an example and it looks like I wasn't paying full attention when I was writing it. We probably should refer to bool variable to make it not a dead code so then it doesn't matter whether you are testing GenASM or GenBCode.

Also, is "control flow that mirrors lexical structure" relevant in this example? Do we get more concise bytecode as a result? I can attribute shorter bytecode you posted above only to DCE.

Contributor

magarciaEPFL commented Jun 5, 2013

is "control flow that mirrors lexical structure" relevant in this example? Do we get more concise bytecode as a result?

Not directly because of it, but that structure improves readability; without hurting any optimizations nor increasing code size.

Compare with the CFG that GenICode produces, which appears to make DCE too difficult for -optimize to get rid of the useless:

    ALOAD 1
    IFNONNULL L1
   L1

(useless because, whether the conditional jump is taken or not, L1 is the target anyway)

Member

gkossakowski commented Jun 5, 2013

Ok. Thanks for confirming that the second optimization changes only the order of blocks without collapsing anything. I understand everything now.

Contributor

magarciaEPFL commented Jun 6, 2013

The discussion about bytecode-test-example can be continued at https://issues.scala-lang.org/browse/SI-7560

Contributor

magarciaEPFL commented Jun 6, 2013

@retronym

More details on how the new backend fares wrt SI-6288 (Wrong line number information in bytecode) for the example of #1754

Summary: GenBCode doesn't experience the line-number confusion described in that PR. The more precise positions that PR introduced are conveyed cleanly in bytecode.

After reproducing the scenario about the breakpoint on the last clause of a case, I can confirm it's not being hit on the way out from other clauses.

That's a consequence from the line-number information emitted by GenBCode. In the example

object Case3 {                                 // 01
  def unapply(z: Any): Option[Int] = Some(-1)  // 02
  def main(args: Array[String]) {              // 03
    ("": Any) match {                          // 04
      case x : String =>                       // 05 Read: <linenumber> JUMP <target basic block id>
        println("case 0")                      // 06 expecting "6 JUMP 7", was "8 JUMP 7"
      case _ =>                                // 07
        println("default")                     // 08 expecting "8 JUMP 7"
    }                                          // 09
    println("done")                            // 10
  }
}

The body of the first case-clause is tagged with LINENUMBER 6 as desired:

   L3
    LINENUMBER 6 L3
    GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
    LDC "case 0"
    INVOKEVIRTUAL scala/Predef$.println (Ljava/lang/Object;)V

And so on so forth.

It's a non-issue with the new backend.

Contributor

magarciaEPFL commented Jun 6, 2013

@retronym Beyond the example from #1754 , it can be seen that GenBCode will fare better than GenICode at preserving line number information from its original, lexical scope based, representation down to bytecode, precisely because GenBCode lays out bytecode ... following lexical scope. A nice overview (in the context or program analysis, but the description of the code layout approach is worth it):

The Program Structure Tree: Computing Control Regions in Linear Time (1994)
by Richard Johnson , David Pearson , Keshav Pingali
http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.38.4257

@magarciaEPFL magarciaEPFL SI-5031 fixed in GenBCode
The GenASM-based fix for SI-5031 is 0527b25
94297df
Contributor

magarciaEPFL commented Jun 6, 2013

@adriaanm

Regarding pos/t5031_3, it seems this should compile. What's the difference under GenBCode?

Sure it should compile, it's a valid Scala program. What worried me was the assertion that was weakened in GenASM as part of the fix. Anyway, in the commit just added that bug is fixed in GenBCode, without weakening any assertion.

With that, all of the tests that run only for GenASM are due to bugs unrelated to GenBCode, bugs for which tickets have been opened.

Owner

adriaanm commented Jun 6, 2013

Ok, great!

Contributor

magarciaEPFL commented Jun 7, 2013

Adding to the status in a previous comment #2620 (comment) , additional issues that were raised and addressed:

This PR should be merged, based on its technical merit, and the detailed feedback provided to reviewers all along.

Owner

retronym commented Jun 9, 2013

LGTM. The process so far has been helpful to educate the reviewers, and to polish the code. Fellow reviewers: WDYT?

Contributor

magarciaEPFL commented Jun 9, 2013

Thanks to all reviewers for the work so far.

A small remark: to catch up with master, in particular the GenASM update in ea7cde1 , a minor update will be needed in getSuperInterfaces() at https://github.com/magarciaEPFL/scala/commit/22ee2df1a58fbdfe2fa8344e4b6658745bc60d17#L10R337

Owner

adriaanm commented Jun 10, 2013

Fair enough -- our other back-ends are also under-tested. I still would like new developments to raise the bar on all counts (testing and other good software engineering practices are also part of the technical merit), so as not to be forever stuck at this level of testiness.

I do want to see continued investment in dbuild-based testing and comparative A/B testing with the original back-end. A summarized diff of compiling the library with GenASM and GenBCode would be quite illuminating.

I'm okay merging this PR once the other reviewers sign off.

For follow-up PRs that introduce new features (not just the ones that fix bugs), tests will be strictly mandatory.
For example, dead-code elimination can easily be tested by comparing the compilation of the same program with and without optimizations and testing that the bytecode is shorter. Clearly, more targeted tests could be devised as well.

Member

gkossakowski commented Jun 10, 2013

I'll have final look this week some time but I cannot promise any dates since we are all busy with Scala Days.

When it comes to testing I agree with @adriaanm.
If we are talking about particular components (like DCE) it should really be done as a set of unit tests. Preferably, just JUnit tests.

Contributor

dragos commented Jun 15, 2013

I'd like to have a look at this PR as well, hopefully I'll have some time these days. I'm not very worried about correctness (in my experience bugs in the backend very rarely end up passing verification, or even not crashing the compiler -- meaning they fail early), but I'm more worried about line numbers. They really influence the experience in the debugger (or the usefulness of stack traces).

@dragos dragos commented on the diff Jun 15, 2013

...er/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+ val Local(tk, _, idx, _) = locals.getOrMakeLocal(s)
+ genLoad(rhs, tk)
+ lineNumber(tree)
+ bc.store(idx, tk)
+
+ case _ =>
+ genLoad(tree, UNIT)
+ }
+ }
+
+ def genThrow(expr: Tree): BType = {
+ val thrownKind = tpeTK(expr)
+ assert(exemplars.get(thrownKind).isSubtypeOf(ThrowableReference))
+ genLoad(expr, thrownKind)
+ lineNumber(expr)
+ emit(asm.Opcodes.ATHROW) // ICode enters here into enterIgnoreMode, we'll rely instead on DCE at ClassNode level.
@dragos

dragos Jun 15, 2013

Contributor

Is this dead-code elimination phase run all the time, or just optimizing? The reason for ignoreMode was that verification would get confused in strange cases where you'd have to build a LUB on incompatible types, like:

 def foo: Int = if (..) ??? else 42

Here, without the ignore mode, there would be a control-flow merge between the two branches, and it would try to find the lub between int and scala.Nothing$ (in JVM terms).

I'm giving this as background info, I guess this test passes now, but keep in mind that DCE is required for correctness now.

@magarciaEPFL

magarciaEPFL Jun 15, 2013

Contributor

@dragos , long time, no see! Great to see you again!

I fully agree (because of that example and others) that DCE is to be performed always.

The previous submission of this PR ( #2603 ) included that must functionality in commit
https://github.com/magarciaEPFL/scala/commit/b0b012e6636263ff982ee4f375ccce7419ed6e25
However, to simplify review, this PR includes only the most basic, barebones, bytecode emitter one can imagine.

To answer your question:

Is this dead-code elimination phase run all the time, or just optimizing?

It runs all the time, not just optimizing. Not as a separate phase, given that DCE is intra-method. It's blazing fast:
https://github.com/magarciaEPFL/scala/commit/b0b012e6636263ff982ee4f375ccce7419ed6e25#L5R82

@dragos dragos commented on the diff Jun 15, 2013

...er/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+ import scalaPrimitives._
+
+ args match {
+ // unary operation
+ case Nil =>
+ genLoad(larg, resKind)
+ code match {
+ case POS => () // nothing
+ case NEG => bc.neg(resKind)
+ case NOT => bc.genPrimitiveArithmetic(icodes.NOT, resKind)
+ case _ => abort(s"Unknown unary operation: ${fun.symbol.fullName} code: $code")
+ }
+
+ // binary operation
+ case rarg :: Nil =>
+ resKind = maxType(tpeTK(larg), tpeTK(rarg))
@dragos

dragos Jun 15, 2013

Contributor

Not sure how expensive is tpeTK, but I mention it just in case: you already computed tpeTK(larg) in resKind.

@dragos dragos commented on the diff Jun 15, 2013

...er/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+ " Call was genLoad" + ((tree, expectedType)))
+
+ case app : Apply =>
+ generatedType = genApply(app, expectedType)
+
+ case ApplyDynamic(qual, args) => sys.error("No invokedynamic support yet.")
+
+ case This(qual) =>
+ val symIsModuleClass = tree.symbol.isModuleClass
+ assert(tree.symbol == claszSymbol || symIsModuleClass,
+ s"Trying to access the this of another class: tree.symbol = ${tree.symbol}, class symbol = $claszSymbol compilation unit: $cunit")
+ if (symIsModuleClass && tree.symbol != claszSymbol) {
+ generatedType = genLoadModule(tree)
+ }
+ else {
+ mnode.visitVarInsn(asm.Opcodes.ALOAD, 0)
@dragos

dragos Jun 15, 2013

Contributor

Don't we need a lineNumber call here?

@dragos dragos commented on the diff Jun 15, 2013

...er/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+ case Ident(name) =>
+ val sym = tree.symbol
+ if (!sym.isPackage) {
+ val tk = symInfoTK(sym)
+ if (sym.isModule) { genLoadModule(tree) }
+ else { locals.load(sym) }
+ generatedType = tk
+ }
+
+ case Literal(value) =>
+ if (value.tag != UnitTag) (value.tag, expectedType) match {
+ case (IntTag, LONG ) => bc.lconst(value.longValue); generatedType = LONG
+ case (FloatTag, DOUBLE) => bc.dconst(value.doubleValue); generatedType = DOUBLE
+ case (NullTag, _ ) => bc.emit(asm.Opcodes.ACONST_NULL); generatedType = RT_NULL
+ case _ => genConstant(value); generatedType = tpeTK(tree)
+ }
@dragos

dragos Jun 15, 2013

Contributor

lineNumber? In case we have multi-line definitions, this might be needed:

val x = "Hello, " +
  "world"
@magarciaEPFL

magarciaEPFL Jun 15, 2013

Contributor

@dragos , I haven't tried them yet, but I can see the debugging experience improving with the additional lineNumbers in question. This is just an example of how GenBCode allows seeing at all times the correspondence:

  • AST node being emitted (Literal in this case, This in the previous comment)
  • bytecode layout

I'm all for adding as many lineNumber as necessary to improve the debugging experience (in fact, it's never "wrong" to include additional lineNumbers). So far I've included those deemed useful, but this is by no means exhaustive. Feel free to add lineNumbers !!!. My suggestion in this regard is: let's not delay this PR just because of those improvements, which can be most easily done on the spot, after this PR has been merged.

Contributor

magarciaEPFL commented Jun 18, 2013

@dragos

I'd like to have a look at this PR as well, hopefully I'll have some time these days.

In case any questions remains, please let me know, so as to answer them.

Is introducing isSuper method and overriding it any better than performing an instanceOf of check?

Owner

magarciaEPFL replied Jun 27, 2013

Thanks for the response. I tend to disagree with readability. The reason is that instanceOf check is very direct. With method call like isSuper you need to grep through all subclasses to check who overrides that method in order to understand the cod you are reading. If there are just two cases (like in isSuper) then I prefer the direct style.

However, as you say, we already use this pattern a lot so I'm fine sticking to it for consistency.

I think we need a better name for this setting. There are several issues with current setting:

  • we introduce unprefixed flag: those are intended to stay long-term and be supported (I don't think that the intention of the neo flag)
  • the name is not very informative

Actually, is there any reason for not just using -YGenBCode flag for the purpose of enabling the alternative backend?

Owner

magarciaEPFL replied Jun 27, 2013

How about -Ybackend:... As before, ... can be used to indicate GenASM (the default), GenBCode, and in the future an optimization level for the new optimizer (which implies GenBCode as bytecode emitter).

Sounds good to me.

I find it confusing that we still have icode phase but it does nothing in case GenBCode is enabled. Why phase assembly doesn't depend on isBCodeActive value so it assembles the right phases?

Owner

magarciaEPFL replied Jun 27, 2013

It's a long story, details in scala#2620 (comment)

The e6f10b0 claims to fix this. Is this work-around still needed?

Owner

magarciaEPFL replied Jun 27, 2013

Great that you spotted this one as well. Same comment as before: I'll remove the defensive code after confirming it's not needed.

Do you really need to force infos explicitly like that? Can you explain why this is needed?

Owner

magarciaEPFL replied Jun 27, 2013

It's a good question, a question that also applies to GenASM (same forcing of infos there). For the time being, GenBCode doesn't aim at deviating from GenASM in that regard. That way, it's easier to compare the output of GenBCode and GenASM. Rather than lumping that change with GenBCode, opening an improvement ticket seems advisable (so that the improvement, if any, is also applied to GenASM).

I wonder if splitting methods that mutate global (thus must be single threaded) into separate class/trait is not a good idea? Did you consider that?

This way one could easier depend on thread safe subset of those methods by sticking to the right type.

Owner

magarciaEPFL replied Jun 27, 2013

That can be done, and would simplify things assuming (say) just the object implementing the multi-thread-safe facet were available. In practice however, GenBCode needs access to both non-thread-safe and multi-thread-safe functionality, sometimes even in the same expression.

I don't see a benefit from breaking up traits on providers along the lines of thread-safety, the clients would also need to be broken up similarly. We would end up with a message-passing metaphor in GenBCode to communicate between clients. That would be too unwieldy in itself.

The e6f10b0 claims to fix this. Is this work-around still needed?

Owner

magarciaEPFL replied Jun 27, 2013

Thanks for the heads up on this. I'll check whether the defensive code isn't run anymore. In case it isn't, I'll remove the defensive code. For the purposes of this PR however, the current behavior isn't wrong.

What this magic constants means? How was it calculated?

Why this logic needs to be executed in cleanup phase and not in genbcode phase?

Member

gkossakowski commented Jun 25, 2013

@magarciaEPFL: I finally had enough time to go through your code in more detail. I really like it overall.

Could you please address questions/suggestions I made?

I think at this point it will be better if you address those points by adding new commits (not amending/rebasing) so I can verify that problems got resolved. If you rebase/amend/squash commits then I have to review the whole code base from scratch again. That would be pleasant given the amount of code you are submitting :)

Once my questions/suggestions are addressed we should merge this in my opinion.

Contributor

magarciaEPFL commented Jun 27, 2013

@gkossakowski , thanks for the review.

Below comments can be found addressing points raised by the review, which comprises three areas:

  1. clarifications ( https://github.com/magarciaEPFL/scala/commit/e8d1612de49418034cc319508b5f6bcb5a7367e6 )
  2. phase assembly ( https://github.com/magarciaEPFL/scala/commit/b6f3611b3e30c1203e9f429a351eaa98864675c7 )
  3. catching up with what has happened in master since this PR was submitted ( https://github.com/magarciaEPFL/scala/commit/af6d1ec68ec3d09c8f9a3097be0026d9804f83a0 , https://github.com/magarciaEPFL/scala/commit/334bc7630138ec3e4e64669678007b5e4b1f36f3 )

To simplify the discussion, I've included candidate commits for the above areas for now in a separate branch. The candidate commits in question are listed again below for summary:
https://github.com/magarciaEPFL/scala/compare/backendish33...backendish33-d

The single commit for item 1 ("clarifications") just adds comments to the source code.

The commits for item 3 are also well-focused, and correspond to a GenASM fix ( https://github.com/magarciaEPFL/scala/commit/334bc7630138ec3e4e64669678007b5e4b1f36f3 ) and a performance improvement ( https://github.com/magarciaEPFL/scala/commit/af6d1ec68ec3d09c8f9a3097be0026d9804f83a0 ).

Contributor

magarciaEPFL commented Jun 27, 2013

Item 2 ("phase assembly", ie what computeInternalPhases() does) isn't as simple as it might look at first. The resulting set of standard phases has remained constant during the following ages:

a. GenJVM only
b. co-existence of GenJVM and GenASM
c. GenASM only

Now comes the question how to assemble phases in the age of "co-existence GenASM + GenBCode". There are two approaches: (i) and (ii) below.

i. The approach I've followed so far, which scales all the way to the new optimizer with all its optimization levels; keeps phase assembly as it is now. Please notice that this "status quo" also includes phases that see no actual use for some runs (the optimizer phases). In this sense, the approach of having the GenICode phase (doing nothing) when using GenBCode as bytecode emitter is no different from the current situation.

ii. A "more clean" approach consists in assembling only those phases that are going to be actually used. In case of using GenBCode, that means leaving out GenICode, the old optimizer, and GenASM. Other scenarios include using (GenICode + GenASM only), or using (GenICode + some-subset-of-optimization-phases + GenASM).

Approach (ii) has a fair amount of ripple effects and additional complexity, as detailed below. In my view, that additional complexity (useful only during the age of "co-existince of GenASM + GenICode") isn't justified. Instead, if all goes well, once we get rid of GenICode + old optimizer + GenASM, then we can also simplify phase assembly.

What is the additional complexity of approach (ii) ?

  [partest] !! 255 - neg/t6446-missing                         [output differs]
  [partest] !! 332 - neg/t7494-no-options                      [output differs]
  [partest] !! 649 - neg/t6446-additional                      [output differs]
  [partest] !! 706 - neg/t6446-show-phases.scala               [output differs]
  [partest] !! 1059 - run/programmatic-main.scala               [output differs]

It's only five tests for the compiler, but it's safe to assume the IDE, sbt, other frameworks, are all showing phases in some of their tests.

All things considered, my suggestion is to live during the transition period with what has worked so well under GenJVM, the transition to GenASM, and GenASM; and change phase assembly only after GenASM is gone (by which time there will be again no need for separate test sets depending on bytecode emitter).

Member

soc commented Jun 27, 2013

instead, it's the need to keep two separate versions of tests for those tests that display phases. In other words, the .check files of the tests below would need different versions for GenASM vs GenBCode

This could be handled by adding support for it as partest filters ... whether is scales, is a different question, though.

Contributor

magarciaEPFL commented Jun 27, 2013

Regarding SI-5604, the defensive code that used to rescue compilation has been made "unreachable" after the fix for the root cause (fix in https://github.com/magarciaEPFL/scala/commit/e6f10b0 ) as pointed out by @gkossakowski .

In order to test the waters without that defensive code, commit https://github.com/magarciaEPFL/scala/commit/2cba0775699730a595d102029897ead4b19b52f9 turns that defensive code into an abort() with descriptive message (although there's already a test, it never hurts to have an additional assert in the compiler thus preventing regressions).

Running the nightly (for each of GenASM, GenBCode) doesn't trigger the abort() in question.

It appears more clear to me to merge this PR as is (defensive code for SI-5604 included) and only afterwards rephrase that defensive code (as in https://github.com/magarciaEPFL/scala/commit/2cba0775699730a595d102029897ead4b19b52f9). @gkossakowski , if you agree then I'll open a ticket (and assign to me) to keep track of this.

Member

gkossakowski commented Jun 27, 2013

Hi Miguel,

useful only during the age of "co-existince of GenASM + GenICode"

That age will span over at least 2 years because even if we switch to new backend in 2.12 we won't be able to remove the old one immediately but only deprecate it.

Therefore the effort to cleanup the phase assembly is worth it. The tests dumping all phases are brittle by definition and should be fixed. I don't think either Sbt or IDE will break because of the change phases. Also, by default the assembled phases will stay intact for now.

I understand that it might feel unfortunate that you have to clean up code that is not really related to new backend but that's reality: whenever you do something significant you discover some unexpected problems that you have to fix.

Also, please push magarciaEPFL@e8d1612, magarciaEPFL@af6d1ec and magarciaEPFL@334bc76 to this branch. I think all of them look fine and should be included as they address some of my review questions.

To sum it up, I think the phase situation is the last remaining todo item on your list. Once this is addressed I think we should merge this PR.

Member

gkossakowski commented Jun 27, 2013

It appears more clear to me to merge this PR as is (defensive code for SI-5604 included) and only afterwards rephrase that defensive code (as in magarciaEPFL@2cba077). @gkossakowski , if you agree then I'll open a ticket (and assign to me) to keep track of this.

Sounds good to me.

Contributor

magarciaEPFL commented Jun 27, 2013

The ticket for the fallout from SI-5604 is https://issues.scala-lang.org/browse/SI-7621 . I'll fix that after this PR is merged.

Member

gkossakowski commented Jun 27, 2013

Actually, I'll fix the phase business. I created the ticket for that and I assigned it to myself: https://issues.scala-lang.org/browse/SI-7622

Please push magarciaEPFL@e8d1612, magarciaEPFL@af6d1ec and magarciaEPFL@334bc76 to this PR and we are ready to go! :-)

@magarciaEPFL magarciaEPFL GenBCode: Eliminate needless Options
This commit brings GenBCode in line with "Eliminate needless Options"
as promoted by 45d6177
ed7f488
Contributor

magarciaEPFL commented Jun 27, 2013

As requested, I've added commits. Please notice magarciaEPFL/scala@ed7f488 will compile only against master, because it uses existingSymbols as defined in https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Symbols.scala#L3418

Member

gkossakowski commented Jun 27, 2013

LGTM.

@adriaanm, @JamesIry: shall we merge?

Owner

adriaanm commented Jun 28, 2013

Let's!

Contributor

JamesIry commented Jul 1, 2013

+1

Owner

adriaanm commented Jul 1, 2013

Yay! 🍰

@adriaanm adriaanm added a commit that referenced this pull request Jul 1, 2013

@adriaanm adriaanm Merge pull request #2620 from magarciaEPFL/backendish33
new bytecode emitter, GenBCode (11th attempt)
e2fbbb2

@adriaanm adriaanm merged commit e2fbbb2 into scala:master Jul 1, 2013

1 check passed

default pr-checkin-per-commit Took 77 min.
Details
Member

gkossakowski commented Jul 1, 2013

Good work! 👏

Owner

adriaanm commented Jul 1, 2013

Yes, thank you @magarciaEPFL and all reviewers. Good stuff!
Now let's get this baby tested and optimized.

hrj commented Jul 19, 2013

Here's a 🍰 from user-land.

Great work all!

@retronym retronym commented on the diff Aug 22, 2013

src/compiler/scala/tools/nsc/backend/jvm/BCodeGlue.scala
+ var i = 0
+ while (i < argumentTypes.length) {
+ argumentTypes(i).getDescriptor(buf)
+ i += 1
+ }
+ buf.append(')')
+ returnType.getDescriptor(buf)
+ buf.toString()
+ }
+
+ } // end of object BType
+
+ /*
+ * Based on ASM's Type class. Namer's chrs is used in this class for the same purposes as the `buf` char array in asm.Type.
+ *
+ * All methods of this classs can-multi-thread
@retronym

retronym Aug 22, 2013

Owner

@magarciaEPFL Could you please elaborate on the interaction between BCode and the name table? I find it somewhat disturbing the chrs is not a private member of Names and that we allow outsiders like this code to peer into it directly.

@magarciaEPFL

magarciaEPFL Aug 22, 2013

Contributor

@retronym In the new backend (ie bytecode emitter + optimizer, GenBCode + BCodeOpt for short) all uses of Namer's chrs are read-only.

Besides that use, the new backend enters via newTypeName the JVM-level internal name of a bytecode-level type (eg java/lang/Object or [Ljava/lang/String; ) this in turn is done for interning: the resulting index to chrs follows a one-to-one correspondence with the "bytecode-level type" in question (a BType instance represents one such "bytecode-level type").

In turn, having one-to-one indices allows BType to become a value class (that's part of upcoming commits, not yet merged from http://magarciaepfl.github.io/scala/ )

If not Namers, any other table providing interning would do. The current write-use of Namers (via newTypeName) by the new backend is single-threaded (there's only one typer-dependent thread in all of GenBCode + BCodeOpt, newTypeName is only be invoked from there).

Both GenBCode and BCodeOpt perform concurrent read-only accesses to Namer's chrs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment