Check field referential equality in autoclonetype #1047

ducky64 · 2019-03-22T02:02:14Z

Check field referential equality in autoclonetype, and check type equivalence in all cases.
We should work on getting #909 merged; the current recommended workaround is to pass Chisel types as Bundle arguments to prevent accidental capture.

Related issue: Fixes #1038, closes #908 (supersedes)

Type of change: bug fix

Impact: API modification (the check is stricter, but if it fails this check your code was broken anyways)

Development Phase: implementation

Release Notes

…ivalence in all cases

ducky64 · 2019-03-22T02:04:42Z

@grebe this supersedes #908, right?

chick · 2019-03-22T03:45:19Z

@ducky64 grebe just left on vacation for two weeks

edwardcwang · 2019-03-22T05:37:44Z

src/test/scala/chiselTests/AutoClonetypeSpec.scala

+        val io = IO(Output(new Bundle {
+          val a = bundleFieldType
+        }))
+      } }


Is the following also subject to issues related to aliased fields?

new Module { val io = IO(Output(new Bundle { val a = UInt(8.W) })) }

Shouldn't, because it creates a new UInt on the constructor invocation, and auto-clone-type constructs a new Bundle each time.

So the following would also have aliasing issues?

class AliasedBundle(foo: UInt) extends Bundle { val a = foo val b = foo }

Yes, but I don't think that's in the domain of auto-cloneType to check, and under current code it will probably cause a rebinding error anyways.

What I don't understand is why autoclonetype can't check for AliasedBundle when it can check for the example in your test...?

It's not a automatically generated clone type issue (as in, issue between the original bundle and cloned bundle), it's your Bundle definition is wrong (problem is solely within one Bundle object). And I think it's also caught by the rebinding checks. If you want I'll add a test case for that?

Sounds good

jackkoenig · 2019-03-22T20:26:00Z

chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala

-            autoClonetypeError(s"automatically cloned $clone not type-equivalent to base." +
-            " Constructor argument values were not inferred, ensure constructor is deterministic.")
-          }
+          validateClone(clone, "Constructor argument values were not inferred, ensure constructor is deterministic.")


Why not run this on creation of elements? That will always happen on or before cloning

@jackkoenig does #1050 address this concern?

ducky64 · 2019-03-22T21:34:10Z

Added a test for aliasing from an inadequate cloneType. Please review this PR only in the scope of aliasing between an original and a clone.

I'm working on aliasing within a single Bundle, but it appears that's an explicitly ignored case in the Bundle element detection logic. I'll PR that separately, because it redefines a semi-explicitly ignored case and probably merits more discussion.

src/test/scala/chiselTests/AutoClonetypeSpec.scala

…ctRefEq

edwardcwang

LGTM, check with @jackkoenig regarding his concern

jackkoenig · 2019-03-25T20:30:22Z

chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala

+            " In the future, this can be solved by wrapping the field in Field(...)," +
+            " see https://github.com/freechipsproject/chisel3/pull/909." +
+            " For now, ensure Chisel types used in the Bundle definition are passed through constructor arguments," +
+            " or wrapped in Input(...), Output(...), or Fliuped(...) if appropriate.")


Slight typo, "Flipped"

* Move Bits, Data, and BitPat to chiselFrontend/src/main/scala/chisel3 and deal with the subsequent fallout. * Move Aggregate, Clock, Mem, Printf, Reg * Move almost all chisel3.core definitions to chisel3 or chisel3.experimental * Revive core package object to provide aliases for moved definitions. * Cleanup package definitions; eliminate ambiguous implicits * Move more definitions to experimental. Extract BaseModule, DataMirror, ExtModule, IO into their own files. * Put BitPat back in chisel3.util * More experimental motion - avoid multiple import definitions. * Add experimental.FixedPoint alias * Add EnumType definition to core package. Update deprecated messages to refer to correct object * Move FixedPoint into the experimental package (but keep it in Bits.scala). * Add missing implicits to core/package - compatibility * Cleanup: update ScalaDoc references; remove unused imports * Add Reset alias to core/package * Use common 3.2 version in deprecation warning * Move Binding from core to internal. * Optimize imports. * Repair IntelliJ's overly cleanliness. * Move Bits, Data, and BitPat to chiselFrontend/src/main/scala/chisel3 and deal with the subsequent fallout. Move Aggregate, Clock, Mem, Printf, Reg Move almost all chisel3.core definitions to chisel3 or chisel3.experimental Revive core package object to provide aliases for moved definitions. Cleanup package definitions; eliminate ambiguous implicits Move more definitions to experimental. Extract BaseModule, DataMirror, ExtModule, IO into their own files. Add EnumType definition to core package. Update deprecated messages to refer to correct object Move FixedPoint into the experimental package (but keep it in Bits.scala). Add missing implicits to core/package - compatibility Cleanup: update ScalaDoc references; remove unused imports Use common 3.2 version in deprecation warning Move Binding from core to internal. * Change == to reference equality (eq) in Data print (#1044) * Remove @chiselName from MixedVec (#1045) * Fix enum annotations (#936) * Turned off strong enum annotations because they weren't working with Vec indexes * Add new EnumVecAnnotation for vecs of enums and vecs of bundles with enum fields * Changed Clock's width parameter back to a fixed constant value of 1 * Fixed enum annotations for Vecs of Bundles which contain enum elements * Fixed usage of "when/otherwise" to use consistent style * Add Record to type hierarchy documentation * Undeprecate isLit (#1048) * move doNotDedup to experimental (#1008) * Aggregate coverage - aggregate tests but not publishing (#1040) Discover a working combination of aggregate usage to enable coverage of subproject testing but publish a single Jar. Use "scalastyle-test-config.xml" for scalastyle config in tests. Enable "_" in method names and accept method names ending in "_=". Re-sync scalastyle-test-config.xml with scalastyle-config.xml This should finally fix #772. * Check field referential equality in autoclonetype (#1047) * Allow naming annotation to work outside builder context (#1051) * Try to eliminate JVM hang due to static initialization deadlock (#1053) * Make core.DontCare private to chisel3 (#1054) Force clients to access 'DontCare' through the chisel3 package to ensure it's created as a chisel3 object and not a client object. * Ignore empty aggregates elements when binding aggregate direction (#946) Previously, including an empty aggregate in a Bundle would cause a MixedDirectionAggregateException because it has no elements and thus doesn't have a direction * Add SampleElementBinding for Vec sample elements * Add ActualDirection.Empty for bound empty aggregates * Detect bundle aliasing (#1050) * Implement connectFromBits in ChiselEnum (#1052) This is necessary to use ChiselEnum in aggregates where things are casted using .asTypeOf * Optimize imports. * Move Analog to experimental. * More repackage cleanup - reduce differences with master. * Cleanup chisel3 references. * More chisel3 reference cleanup. * Merge cleanup. * Remove unused import * Bump core deprecation to 3.3 * Move DontCare back into Data.scala inside package internal * Re-indent experimental/internal package code * Move code back to original files - facilitate comparison with other branches * Some code motion, update imports, minimize master differences Move exceptions up to chisel3 package object - they're part of the interface. * More master diff minimization. * Try to eliminate JVM hang due to static initialization deadlock (#1053) * Ignore empty aggregates elements when binding aggregate direction (#946) Previously, including an empty aggregate in a Bundle would cause a MixedDirectionAggregateException because it has no elements and thus doesn't have a direction * Add SampleElementBinding for Vec sample elements * Add ActualDirection.Empty for bound empty aggregates * Implement connectFromBits in ChiselEnum (#1052) This is necessary to use ChiselEnum in aggregates where things are casted using .asTypeOf * Move Analog to experimental. More repackage cleanup - reduce differences with master. Cleanup chisel3 references. More chisel3 reference cleanup. * Fix wrong directionality for Vec(Flipped()) Create Chisel IR Port() in a way that Converter is happy with. Also add more extensive test suite for future-proofing. Close #1063 * Move Bits, Data, and BitPat to chiselFrontend/src/main/scala/chisel3 and deal with the subsequent fallout. Move Aggregate, Clock, Mem, Printf, Reg Move almost all chisel3.core definitions to chisel3 or chisel3.experimental Revive core package object to provide aliases for moved definitions. Cleanup package definitions; eliminate ambiguous implicits Move more definitions to experimental. Extract BaseModule, DataMirror, ExtModule, IO into their own files. Put BitPat back in chisel3.util More experimental motion - avoid multiple import definitions. Add experimental.FixedPoint alias Add EnumType definition to core package. Update deprecated messages to refer to correct object Move FixedPoint into the experimental package (but keep it in Bits.scala). Add missing implicits to core/package - compatibility Cleanup: update ScalaDoc references; remove unused imports Add Reset alias to core/package Use common 3.2 version in deprecation warning Move Binding from core to internal. Optimize imports. Repair IntelliJ's overly cleanliness. Move Bits, Data, and BitPat to chiselFrontend/src/main/scala/chisel3 and deal with the subsequent fallout. Move Aggregate, Clock, Mem, Printf, Reg Move almost all chisel3.core definitions to chisel3 or chisel3.experimental Revive core package object to provide aliases for moved definitions. Cleanup package definitions; eliminate ambiguous implicits Move more definitions to experimental. Extract BaseModule, DataMirror, ExtModule, IO into their own files. Add EnumType definition to core package. Update deprecated messages to refer to correct object Move FixedPoint into the experimental package (but keep it in Bits.scala). Add missing implicits to core/package - compatibility Cleanup: update ScalaDoc references; remove unused imports Use common 3.2 version in deprecation warning Move Binding from core to internal. Optimize imports. Merge cleanup. Remove unused import Bump core deprecation to 3.3 Move DontCare back into Data.scala inside package internal Re-indent experimental/internal package code Move code back to original files - facilitate comparison with other branches Some code motion, update imports, minimize master differences Move exceptions up to chisel3 package object - they're part of the interface. More master diff minimization. Fix minor discrepancies with repackagecore-testbed * Remove redundant imports As part of its import updating process, IntelliJ converted some import statements to `import package.{object, _}`. Is this intended to show an explicit dependency on `package.object` and a further dependency on `package` implicits? Unsure. Replace these with `import package._` * Move the BaseModule object into the internal package.

ducky64 added 2 commits March 21, 2019 18:47

Check field referential equality in autoclonetype, and check type equ…

8e763f2

…ivalence in all cases

better diagnostics

53a3b36

ducky64 requested a review from a team as a code owner March 22, 2019 02:02

ducky64 requested a review from edwardcwang March 22, 2019 02:04

edwardcwang reviewed Mar 22, 2019

View reviewed changes

jackkoenig reviewed Mar 22, 2019

View reviewed changes

ducky64 and others added 2 commits March 22, 2019 14:13

add alias from inadequate autoclonetype

450a708

Merge branch 'master' into actRefEq

f80d719

edwardcwang reviewed Mar 22, 2019

View reviewed changes

src/test/scala/chiselTests/AutoClonetypeSpec.scala Outdated Show resolved Hide resolved

ducky64 added 2 commits March 22, 2019 14:35

fix

f814b91

Merge branch 'actRefEq' of github.com:freechipsproject/chisel3 into a…

521153d

…ctRefEq

ducky64 mentioned this pull request Mar 22, 2019

Detect bundle aliasing #1050

Merged

edwardcwang approved these changes Mar 22, 2019

View reviewed changes

Merge branch 'master' into actRefEq

b2e0da5

jackkoenig reviewed Mar 25, 2019

View reviewed changes

ducky64 added 2 commits March 25, 2019 13:34

Merge branch 'master' into actRefEq

cac7b4c

lol fix typo

a03839f

ducky64 merged commit 1f8a586 into master Mar 25, 2019

ducky64 deleted the actRefEq branch March 25, 2019 21:04

ucbjrl pushed a commit that referenced this pull request Apr 15, 2019

Check field referential equality in autoclonetype (#1047)

45955a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check field referential equality in autoclonetype #1047

Check field referential equality in autoclonetype #1047

ducky64 commented Mar 22, 2019 •

edited

Loading

ducky64 commented Mar 22, 2019

chick commented Mar 22, 2019

edwardcwang Mar 22, 2019

ducky64 Mar 22, 2019

edwardcwang Mar 22, 2019

ducky64 Mar 22, 2019

edwardcwang Mar 22, 2019

ducky64 Mar 22, 2019

edwardcwang Mar 22, 2019

jackkoenig Mar 22, 2019

ducky64 Mar 23, 2019

ducky64 commented Mar 22, 2019

edwardcwang left a comment

jackkoenig Mar 25, 2019

Check field referential equality in autoclonetype #1047

Check field referential equality in autoclonetype #1047

Conversation

ducky64 commented Mar 22, 2019 • edited Loading

ducky64 commented Mar 22, 2019

chick commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 commented Mar 22, 2019

edwardcwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 commented Mar 22, 2019 •

edited

Loading