Hierarchy API: make Mems lookupable
#2404
Conversation
| if (Builder.currentModule != parent) { | ||
| Builder.currentModule = parent | ||
| } |
There was a problem hiding this comment.
@azidar this was the fix I arrived at to get rid of the "must be inside Builder context" failure, but I am unsure if this is an acceptable fix...
There was a problem hiding this comment.
Sure! Do you know why this check is necessary? I would have thought just setting it would have been ok..
There was a problem hiding this comment.
Hmmm... I am no longer sure this is actually a correct fix. I think the proper thing to do would be to check that parent is not None.
Some tests in InstanceSpec were failing with requirement failed: must be inside Builder context on accesses to dynamicContext when trying to set Builder.currentModule. After some digging, I think the failure is caused by Builder.currentModule returning None b/c it was getting set to parent in cases where parent was None.
| @@ -229,6 +229,7 @@ package internal { | |||
| // Private internal class to serve as a _parent for Data in cloned ports | |||
| private[chisel3] class ModuleClone[T <: BaseModule](val getProto: T) extends PseudoModule with IsClone[T] { | |||
| override def toString = s"ModuleClone(${getProto})" | |||
| override def addId(d: HasId): Unit = () | |||
There was a problem hiding this comment.
This and the other addId override is very non-obvious to me, can you add a comment explaining why?
There was a problem hiding this comment.
// Do not call default addId function, which may modify a module that is already "closed"
(cherry picked from commit 2a985ac)
Contributor Checklist
docs/src?Type of Improvement
API Impact
@publiccan now be used on definitions/instances ofMems andSyncReadMems.Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.4.x, [small] API extension:3.5.x, API modification or big change:3.6.0)?Please Merge?