Sharing GenBcode backend between scalac and dotty #4136
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They are using primitives and opcodes that were extracted to objects.
…ces. Saves allocations but breaks thread-safety of BackendInterface.
|
I cannot reproduce error with which jenkins failed: https://scala-webapps.epfl.ch/jenkins/job/pr-scala-publish-core/4763/console |
|
Did you try |
|
I've tried making a clean clone and building it. |
|
I reproduced it with: |
|
Here's a fix: You must not call into |
ArrayClass is magical. Several methods had recursive implementation. PatternMatching was shadowing parameter.
|
@retronym thanks for help! I tried to run all testsuite with modified genbcode. I have failures for tests such as pos/t5545, but it seems that I'm bug compatible with https://issues.scala-lang.org/browse/SI-8431 |
|
The left failures are one neg and one run. I can reproduce neither of those. I can compile failed tests by hand with self-bootstrapped compiler, while it seems that in Jenkins they fail with NPE. I don't have a stacktrace to see where this NPE was issued, so I'm not sure how to fix it. |
|
The test failure was just a friendly reminder you either to regenerate the source of |
|
@DarkDimius I am also unable to locally reproduce the test failures in After building a merge of d37ad and Let's see if it is deterministic: PLS REBUILD/pr-scala@d37adac374f4127f66ff74e8cac756f6e0f74665 |
|
(kitty-note-to-self: ignore 63910525) |
|
OMG this is definitely going to endanger the cross-compilation of the Scala.js backend against all versions of Scala ... Note that I'm not even speaking about actually using that abstraction. At first glance, at least the changes in |
|
|
||
| if(!int.shouldEmitJumpAfterLabels) genNormalBlock | ||
| else { | ||
| val (prefixLabels: List[LabelDef], stats1) = stats.span { |
There was a problem hiding this comment.
[locker.compiler] /Users/luc/scala/scala2/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala:857: warning: abstract type LabelDef in type pattern List[BCodeBodyBuilder.this.int.LabelDef] (the underlying of List[BCodeBodyBuilder.this.int.LabelDef]) is unchecked since it is eliminated by erasure
[locker.compiler] val (prefixLabels: List[LabelDef], stats1) = stats.span {
[locker.compiler] ^
snapshot scala
| val LongTag: ConstantTag | ||
| val StringTag: ConstantTag | ||
| val ClazzTag: ConstantTag | ||
| val EnumTag: ConstantTag |
There was a problem hiding this comment.
This abstraction breaks the @switch in genConstant:
[quick.compiler] /Users/luc/scala/scala2/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala:459: warning: could not emit switch for @switch annotated match
[quick.compiler] (const.tag: @switch) match {
[quick.compiler] ^
Not specific to this change: We need to do at least some performance testing to get a feeling for the performance penalty before adding the new abstraction layer.
There was a problem hiding this comment.
For this particular abstraction, actual ConstantTag==Long values are same in dotty and scalac, so if this particular place introduces a slowdown it could be solved.
The way how I observe how fast is the compiler is time that is taken to compile standard library. Before 2d22839 it was 10% slower. After I didn't check.
Handling Ident's in dotty requires analysing its type, Instead of exporting a lot of abstractions around types I'd better go with a single method that hides this.
| */ | ||
| for (i <- args.length until dims) elemKind = ArrayBType(elemKind) | ||
| } | ||
| (argsSize : @switch) match { |
There was a problem hiding this comment.
can you remove the @swtich here? it triggers a warning:
[quick.compiler] /Users/luc/scala/scala2/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala:606: warning: matches with two cases or fewer are emitted using if-then-else instead of switch
[quick.compiler] (argsSize : @switch) match {
[quick.compiler] ^
This is a re-run of SI-5158 in two different contexts. As reported, the result of `unapply[Seq]` under name based pattern matching might not guarantee stability of results of calls to `_1`, `apply(i)`, etc, so we call these eagerly, and call them once only. I also found a case in regular pattern matching that we hadn't accounted for: extracting elements of sequences (either from a case class or from an `unapplySeq`) may also be unstable. This commit changes `ExtractorTreeMaker` to force storage of such binders, even under `-optimize`. This parallels the change to `ProductExtractorTreeMaker` in 8ebe8e3. I have added a special case for traditional `unapply` methods returning `Option`. This avoids a change for: ``` % cat test/files/run/t9003b.scala object Single { def unapply(a: Any) = Some("") } object Test { def main(args: Array[String]): Unit = { "" match { case Single(x) => (x, x) } } } % qscalac -optimize -Xprint:patmat test/files/run/t9003b.scala 2>&1 | grep --context=5 get case <synthetic> val x1: Any = ""; case5(){ <synthetic> val o7: Some[String] = Single.unapply(x1); if (o7.isEmpty.unary_!) matchEnd4({ scala.Tuple2.apply[String, String](o7.get, o7.get); () }) else case6() }; % scalac-hash v2.11.4 -optimize -Xprint:patmat test/files/run/t9003b.scala 2>&1 | grep --context=5 get case <synthetic> val x1: Any = ""; case5(){ <synthetic> val o7: Some[String] = Single.unapply(x1); if (o7.isEmpty.unary_!) matchEnd4({ scala.Tuple2.apply[String, String](o7.get, o7.get); () }) else case6() }; ```
needed to handle array_clone in dotty. Which doesn't have a symbol but isn't byte code primitive operation
| @@ -637,7 +644,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters { self => | |||
| def descriptor(k: TypeKind): String = { javaType(k).getDescriptor } | |||
| def descriptor(s: Symbol): String = { javaType(s).getDescriptor } | |||
|
|
|||
| def javaType(tk: TypeKind): asm.Type = { | |||
| def javaType(tk: TypeKinds#TypeKind): asm.Type = { | |||
There was a problem hiding this comment.
are these projections in GenASM still necessary?
DarkDimius
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 12, 2014
DarkDimius
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 15, 2014
DarkDimius
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 16, 2014
|
Let's close this one for now, as it requires a chunk of work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR abstracts GenBcode implementation classes from existence of global using forwarders, which would have a different implementation in dotty.
The main class here is BackendInterface that defines everything that backend uses.
While it would be good to have backend use less that 150 methods on
Symbolthis commit doesn't address this issue: it tries to not introduce any changes in behavior.In order to not loose perfromance and not allocate options during pattern matching this implemtation relies on name-based pattern matching with preallocated objects, this makes BackendInterface non-thread safe, but as of now it is used only in single thread. If several threads will ever be used to transfrom AST to asm bytecode than each of threads will need to own its own instance of BackendInterface.
@lrytz please have a look.