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 deprecation warning that leaks into user code #1256

Merged
merged 2 commits into from
Nov 30, 2019

Conversation

jackkoenig
Copy link
Contributor

Data.isLit called Data.litArg which would trigger a Chisel runtime
deprecation warning in user code with source locator Data.scala:488

As reported in https://stackoverflow.com/questions/59049673/how-to-initialize-a-reg-of-bundle-in-chisel/59058082?noredirect=1#comment104418728_59058082

Related issue:

Type of change: bug fix

Impact: no functional change

Development Phase: implementation

Release Notes
Fix internal deprecation warning on Data.isLit that leaked into user code

Data.isLit called Data.litArg which would trigger a Chisel runtime
deprecation warning in user code with source locator Data.scala:488
@jackkoenig jackkoenig added the Bugfix Fixes a bug, will be included in release notes label Nov 29, 2019
@jackkoenig jackkoenig added this to the 3.2.X milestone Nov 29, 2019
@jackkoenig jackkoenig requested a review from a team as a code owner November 29, 2019 21:04
@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Nov 29, 2019
@jackkoenig
Copy link
Contributor Author

@ucbjrl it seems that Jenkins is down. Did we decide to remove Jenkins from regular CI flow? Should we go ahead and remove it?

@ucbjrl
Copy link
Contributor

ucbjrl commented Nov 30, 2019

Let's remove Jenkins from the PR flow.

@jackkoenig
Copy link
Contributor Author

@ucbjrl can you or one of the other repo admins do it? It's somewhere in the repo settings

@ucbjrl
Copy link
Contributor

ucbjrl commented Nov 30, 2019

I removed it. I'm not sure what else may be required. I'll squash and merge it once the CircleCI checks complete.

@jackkoenig
Copy link
Contributor Author

Thanks Jim!!

@ucbjrl ucbjrl merged commit afef8b8 into master Nov 30, 2019
mergify bot pushed a commit that referenced this pull request Nov 30, 2019
Data.isLit called Data.litArg which would trigger a Chisel runtime
deprecation warning in user code with source locator Data.scala:488

(cherry picked from commit afef8b8)
@mergify mergify bot added the Backported This PR has been backported label Nov 30, 2019
mergify bot added a commit that referenced this pull request Nov 30, 2019
Data.isLit called Data.litArg which would trigger a Chisel runtime
deprecation warning in user code with source locator Data.scala:488

(cherry picked from commit afef8b8)
@jackkoenig jackkoenig deleted the fix-deprecation-warning branch December 6, 2019 20:26
@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 13, 2019

Unfortunately, this breaks dsptools since:

  override def litOption: Option[BigInt] = ???  // TODO implement me
[info] ComplexOpSpecification:
[info] 
[info]   The ParameterizedNumericOperation demonstrates a Module that can be instantiated to
[info]   handle different numeric types and different numerical operations
[info]   
[info]   This instance will process DspComplex[Real] numbers with the basic mathematical operations
Total FIRRTL Compile Time: 93.9 ms
[info]   - operation + should work for all inputs *** FAILED ***
[info]     firrtl.options.OptionsException: Exception thrown when elaborating ChiselGeneratorAnnotation
[info]     at chisel3.stage.ChiselGeneratorAnnotation.elaborate(ChiselAnnotations.scala:55)
[info]     at chisel3.stage.phases.Elaborate$$anonfun$transform$1.apply(Elaborate.scala:19)
[info]     at chisel3.stage.phases.Elaborate$$anonfun$transform$1.apply(Elaborate.scala:18)
[info]     at scala.collection.TraversableLike$$anonfun$flatMap$1.apply(TraversableLike.scala:241)
[info]     at scala.collection.TraversableLike$$anonfun$flatMap$1.apply(TraversableLike.scala:241)
[info]     at scala.collection.immutable.List.foreach(List.scala:392)
[info]     at scala.collection.TraversableLike$class.flatMap(TraversableLike.scala:241)
[info]     at scala.collection.immutable.List.flatMap(List.scala:355)
[info]     at chisel3.stage.phases.Elaborate.transform(Elaborate.scala:18)
[info]     at chisel3.stage.phases.Elaborate.transform(Elaborate.scala:16)
[info]     ...
[info]     Cause: scala.NotImplementedError: an implementation is missing
[info]     at scala.Predef$.$qmark$qmark$qmark(Predef.scala:230)
[info]     at chisel3.Aggregate.litOption(Aggregate.scala:46)
[info]     at chisel3.Data.isLit(Data.scala:496)
[info]     at dsptools.numbers.DspComplex$.apply(DspComplex.scala:22)

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 13, 2019

dsptools just wants to know if this is a lit; it doesn't care about its value. Perhaps we should have isLit() just return true if the binding is a literal?

@jackkoenig
Copy link
Contributor Author

Why doesn't it care about the value? Should we instead implement litOption? DspComplex is a Bundle right? Can we use Bundle literals?

In any case, this may need to be reverted from 3.2.x since it is not compatible without modification to DSP tools

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 13, 2019

It's an optimization. Check out DspComplex.scala

@jackkoenig
Copy link
Contributor Author

Oh I see, I guess alternatively we can just make litOption None for the things that can't be lits...

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 Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants