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

Compat compile options macro #1253

Merged
merged 3 commits into from
Nov 29, 2019
Merged

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Nov 27, 2019

This switches from using an implicit val that required awkward
suppression (as illustrated in CompileOptionsSpec) to allowing
overriding in the same way as done in "import chisel3._" via the
creation of an implicit val in lexical scope.

The motivation is attempting to partially migrate Diplomatic stuff in rocket-chip to import chisel3._. I currently have to do the following:

import Chisel.{defaultCompileOptions => _, _}
import freechips.rocketchip.util.CompileOptions.NotStrictInferReset

It would be nice if we could just write

import Chisel._
import freechips.rocketchip.util.CompileOptions.NotStrictInferReset

This is probably backportable to 3.2.x, although if so we should (probably) exclude the deprecation of Chisel.defaultCompileOptions.

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

  • Replace Chisel.defaultCompileOptions with a low-priority macro
  • Deprecate Chisel.defaultCompileOptions

This switches from using an implicit val that required awkward
suppression (as illustrated in CompileOptionsSpec) to allowing
overriding in the same way as done in "import chisel3._" via the
creation of an implicit val in lexical scope.
@jackkoenig jackkoenig requested a review from ucbjrl November 27, 2019 22:18
@jackkoenig jackkoenig requested a review from a team as a code owner November 27, 2019 22:18
Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

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

lgtm

@jackkoenig jackkoenig merged commit 85fe90d into master Nov 29, 2019
@jackkoenig jackkoenig added the Backported This PR has been backported label Nov 29, 2019
@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 4, 2019

Unfortunately, this seems to cause rocket-chip regression tests to fail:

[info] running freechips.rocketchip.unittest.Generator emulator/generated-src freechips.rocketchip.unittest TestHarness freechips.rocketchip.unittest TLSimpleUnitTestConfig
[info] [0.003] Elaborating design...
[error] (run-main-0) java.lang.reflect.InvocationTargetException
[error] java.lang.reflect.InvocationTargetException
[error] 	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
[error] 	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
[error] 	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
[error] 	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
[error] 	at freechips.rocketchip.util.HasGeneratorUtilities.$anonfun$elaborate$1(GeneratorUtils.scala:55)
[error] 	at chisel3.Module$.do_apply(Module.scala:52)
[error] 	at chisel3.Driver$.$anonfun$elaborate$1(Driver.scala:93)
[error] 	at chisel3.internal.Builder$.$anonfun$build$2(Builder.scala:406)
[error] 	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:62)
[error] 	at chisel3.internal.Builder$.$anonfun$build$1(Builder.scala:404)
[error] 	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:62)
[error] 	at chisel3.internal.Builder$.build(Builder.scala:404)
[error] 	at chisel3.Driver$.elaborate(Driver.scala:93)
[error] 	at freechips.rocketchip.util.HasGeneratorUtilities.elaborate(GeneratorUtils.scala:60)
[error] Caused by: chisel3.package$ExpectedChiselTypeException: 'TLBundleA(Wire in TLRationalCrossingSource)' must be a Chisel type, not hardware
[error] 	at chisel3.internal.requireIsChiselType$.apply(Binding.scala:30)
[error] 	at chisel3.SpecifiedDirection$.specifiedDirection(Data.scala:49)
[error] 	at chisel3.Output$.apply(Data.scala:241)
[error] 	at Chisel.package$AddDirectionToData.asOutput(compatibility.scala:38)
[error] 	at freechips.rocketchip.util.RationalIO.<init>(RationalCrossing.scala:52)
[error] 	at freechips.rocketchip.util.RationalIO$.apply(RationalCrossing.scala:64)
[error] 	at freechips.rocketchip.util.RationalCrossingSource$$anon$1.<init>(RationalCrossing.scala:71)
[error] 	at freechips.rocketchip.util.RationalCrossingSource.<init>(RationalCrossing.scala:69)
[error] 	at freechips.rocketchip.util.ToRational$.$anonfun$apply$1(RationalCrossing.scala:157)
[error] 	at chisel3.Module$.do_apply(Module.scala:52)
[error] 	at freechips.rocketchip.util.ToRational$.apply(RationalCrossing.scala:157)
[error] 	at freechips.rocketchip.tilelink.TLRationalCrossingSource$$anon$1.$anonfun$new$1(RationalCrossing.scala:26)
[error] 	at freechips.rocketchip.tilelink.TLRationalCrossingSource$$anon$1.$anonfun$new$1$adapted(RationalCrossing.scala:22)
[error] 	at scala.collection.immutable.List.foreach(List.scala:392)
[error] Nonzero exit code: 1
[error] (Compile / runMain) Nonzero exit code: 1
[error] Total time: 14 s, completed Dec 4, 2019 3:31:50 PM
make[1]: *** [emulator/generated-src/freechips.rocketchip.unittest.TLSimpleUnitTestConfig.fir] Error 1
make: *** [stamps/TLSimpleUnitTestConfig/emulator-ndebug.stamp] Error 2

@jackkoenig
Copy link
Contributor Author

uh oh. I'll debug

@jackkoenig
Copy link
Contributor Author

@ucbjrl this did not make it into 3.2.1 right?

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 5, 2019

Correct. It's been back-ported to 3.2.x so it would go out in 3.2.2.

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 5, 2019

It looks like the ExplicitCompileOptions (all false) are correct in the target passed to AddDirMethodToData(), but they're replaced by all true in Output.apply().

@jackkoenig
Copy link
Contributor Author

Alright so the issue then is that by removing the implicit val, package Chisel is pulling it from import chisel3._ at the very top of the file.

@jackkoenig
Copy link
Contributor Author

It's worse than that, some parts are using the implicit from the macro, others are pulling from import chisel3._:

This pulls from import chisel3._ thus the error

  implicit class AddDirectionToData[T<:Data](target: T) {
    def asInput: T = Input(target)
    def asOutput: T = Output(target)
    def flip(): T = Flipped(target)
  }

This gets the macro in object Chisel

  object Wire extends WireFactory {
    import chisel3.CompileOptions

    def apply[T <: Data](dummy: Int = 0, init: T)(implicit compileOptions: CompileOptions): T = {
      println(s"compileOptions = $compileOptions")
      chisel3.WireDefault(init)
    }

    def apply[T <: Data](t: T, init: T)(implicit compileOptions: CompileOptions): T =
      chisel3.WireDefault(t, init)
  }

@jackkoenig
Copy link
Contributor Author

I think this is fixable, but I don't have the time to do it thoroughly at the moment so I'm going to revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants