-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
TaggedUnion implementation #1241
base: dev
Are you sure you want to change the base?
Conversation
Nice :D I pushed fixes for the naming and the HardType.unionSeq. About the choose, there is one less verbose alternative syntax which could be : def choose(callback: PartialFunction[Data, Unit]): Unit = {
for((name, k) <- unionDescriptors if callback.isDefinedAt(k)){
val variant = tagUnionDescriptors(name)
this.tag := variant
callback(this.unionPayload.aliasAs(k))
return
}
SpinalError(s"not a member of this TaggedUnion")
}
Which can be used as
rw.choose { case r: WriteRequest =>
r.y := 255
} I think partialfunction it can also be used for the among to produce : rw.among {
case r: ReadRequest => {
// you can read the tagged union as a ReadRequest
... := r
}
case w: WriteRequest => {
// you can read the tagged union as a WriteRequest
... := w
}
} So, to be clear, it isn't a replacement of the original among, as it only rely on type, which may not be enough in all cases. |
Removing explicitly naming the variant in choose and among was my first version but may raise issues : case class SameTypeUnion() extends TaggedUnion {
val a1 = TypeA()
val a2 = TypeA()
} is not usable anymore. But this case could be replaced by case class VariantBundle extends Bundle {
val a = TypeA()
val variant = SpinalEnum() // a1 or a2
} Yet, it implies that TaggedUnion must refuse multiple variants with the same type. case class AlmostSameTypeUnion() extends TaggedUnion {
val a1 = UInt(8 bits)
val a2 = UInt(16 bits)
} implies to distinguish between the two variants inside the |
I have to check if we can propose the two possibilities (with and without explicit variants) |
… are type based only. xxxVariant(s) requires an explicit variant value
Ok, new versions for among and choose :
when(io.doReq) {
io.request.valid := True
when(io.rw) {
io.request.payload.choose {
case wReq: WriteRequest => {
wReq.address := 2
wReq.value := 0
}
}
}
.otherwise {
io.request.payload.chooseVariant(io.request.payload.read) {
rReq: ReadRequest => {
rReq.address := 1
}
}
}
} when(io.response.valid) {
io.response.payload.among {
case rRes: ReadResponse => {
io.answer := rRes.value
}
case wRes: WriteResponse => {
when(wRes.success) {
io.answer := 0
}
.otherwise {
io.answer := 1
}
}
}
} Different method names XXX and XXXVariant are required because the type system has trouble to differentiate them otherwise. |
… use the Hardtype.union method instead of the old Union class
@ronan-lashermes The TaggedUnionTesterCocotbBoot failed ? |
I had to interrupt my work. But I am still working on it as soon as I have a little time (coming week should be ok). |
No worries ^^ |
Ok, tester fixed and first documentation draft proposed.
|
I'm thinking, it would also probably possible to have something like : import scala.reflect.{ClassTag, classTag}
def choose[T <: Data : ClassTag](body : T => Unit) : Unit = ...
myTaggedUnion.choose[MyElementType].xyz probably same for ammong
to be do thing in a more scala way, it could be :
|
Thanks for the suggestions. when(io.rw) {
io.request.payload.update {
wReq: WriteRequest => {
wReq.address := 2
wReq.value := 0
}
}
}
.otherwise {
io.request.payload.update(io.request.payload.read) {
rReq: ReadRequest => {
rReq.address := 1
}
}
} And both among and amongVariants has been merged into a more accepting apply: case class SameTypeUnion() extends TaggedUnion {
val a1 = TypeA()
val a2 = TypeA()
val b = TypeB()
}
val tu = SameTypeUnion()
tu {
case bVal: TypeB => {
// read tu as of TypeB when b variant selected
v := bVal.valid
}
case (tu.a1, aVal: TypeA) => {
//...
}
case (tu.a2, aVal: TypeA) => {
//...
}
}
I still need to write the tests though. |
Cool thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick question (and a nitpick):
- How would I currently
init
a Reg(TaggedUnion) to have a specific reset value? Did I overlook that in the code? - would it be possible to have a way of just comparing the active member of the union in something like
when(myUnion.is(ta.a1))
? I don't see any issues with that (not saying you have to add it now, just whether we can add it in the future)
class TaggedUnion(var encoding: SpinalEnumEncoding = native) extends MultiData with Nameable with ValCallbackRec with PostInitCallback { | ||
|
||
// A cache of union member descriptors, storing a tuple of their name and the corresponding Data object. | ||
var unionDescriptors = ArrayBuffer[(String, Data)]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the looks of it this (and some of the next members) should be val
instead of var
? I don't see us reassigning them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you are right the first 3 vars should be vals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, last commit added the is(data)
function as you mentioned, good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the init
, this is where I may need a little guidance.
Is there a special trait to implement for that ? Where can I find an example ?
Related is the need to assign outside of update
(even if with assignDontCare
) or we get latches.
So I did try to write some tests, but I can’t get it to pass locally. But I get no meaningful log, the error could be a missing dependency for all I know... |
Mostly, you need iverilog, ghdl, cocotb, verilator installed. The doc about those is the CI, unfortunatly. Where things go bad ? |
I still missing dependencies (python packages) for my local tests, but I gradually improve on that. It’s just a bit long with trial and errors.
Still trying to make sense of this error message. |
Ahhh you did you testbench in cocotb ? Have you seen the following for tooling install ? : Line 4 in 68b6158
|
Locally my issue is that cocotb uses find_libpython and can’t find the module even if present. But this is most probably due to my local environment setup which is nix-based (great when it works, else a nightmare...). For the test, yeah I tried to replicate the BundleTester with a cocotb testbench. |
So in general, i moved away from cocotb and now only using SpinalSim :)
Ahhh i don't know nix so well, it is very package oriented right ? (i kinda always try to get things from source)
Yes, and in old version of cocotb the default was that X were translated into 0. from cocotblib.misc import cocotbXHack
cocotbXHack()
SpinalSim <3, else the trick above i would say |
Hi ^^ How things are going ? |
I actually got a working SpinalSim going. Unfortunately, I need a few more days to find the time to iron things out and push the final tester. I also progressed on the Init problem, that may require slight modifications to existing init : the function signature is currently init(hardware: T, value: T), where we need in our case: init(hardware: T, value: V), with both T and V: Data (in TaggedUnion, the type of the union is different than the types of its possible values). |
Hoo right, i haven't thinked about the init situation XD |
…nd init value Data type
Ok, here a new version with a working SpinalSim tester and a first try at init (in 6b15880).
Does that seems like the correct direction for you ? |
Hi ^^ |
For the converter: the issue is with inits such as It works in nearly all cases but a corner case with SpinalEnum (cf Phases.scala). I am still not sure of what implicit converter was called. These corner cases are tied to type system warnings for the same lines in the master branch. The gist of this modification is to be able to use RegInit, accepting any input as long as there is a converter at hand. I will try to improve things a bit (and explore the special TaggedUnion init direction). |
Ok I pushed a new try with a dedicated initVariant to init a TaggedUnion with a variant value in a register.
I convert the variant to the "neutral" unionPayload with aliasAs. But I am not sure that this is equivalent to
The latter cannot work since |
Hmm, i'm not sure to understand why using aliasAs ? Why not this.unionPayload.init(variantData.asBits.resized) ? i may have missed something |
Your solution gives the same Verilog as mine. My line is just a bit more complex ;) unionPayload <= {variant_x,variant_y}; where I would prefer unionPayload[7:0] <= variant_x;
unionPayload[15:8] <= variant_y; The explicit assignation seems to me less error prone. |
As long as it works ^^ Can't merge the branch, as there is merge conflicts |
Ok, I will look into it. And perform some more tests... |
Work-in-progress for #1214
Context, Motivation & Description
This is a first attempt at a TaggedUnion class leveraging the new union feature.
This is not targeting a merge for now, but I would welcome feedback on this implementation.
Requires more tests / polish.
Checklist
/** */
?Usage
A tagged union is a Sum Type. Use it as a Bundle, but a tagged union value is one value among the descriptors.
There are two main methods to deal with tagged unions.
choose
select the union variant.The first argument selects the variant, the second provides an object with the correct variant type for your usage.
among
to "read" the union.Why do you need a 2-tuple in among ? To deal with tagged union with multiple variants of the same type.
Pros/cons
Pros:
Cons:
assignDontCare
that may be hidden inside the TaggedUnion implementation.Longer example
Here is a simple example to describe a read/write port. You should be able to copy/write this example and get it elaborated.