chisel
chisel copied to clipboard
chipsalliance
.suggestName and .autoSeed change the prefix even when they don't change the "local name"
This is a weird one that is almost certainly usually a feature but can give some very strange behavior
Consider the following:
class Example extends Module {
val foo, bar = IO(Input(UInt(8.W)))
val out = {
val port = IO(Output(UInt(8.W)))
port.suggestName("port")
port
}
out := foo + bar
}
(Scastie: https://scastie.scala-lang.org/YzgJRnIIQWGjDcXSBblsVw)
Most would expect the name of the Output port to be port, which is indeed the case.
However, it is not for the reason one might think. .suggestName is actually sensitive to the prefix, consider the following:
class Example extends Module {
val in = IO(Input(UInt(8.W)))
val out = {
val delayed = RegNext(in).suggestName("in_delayed")
val port = IO(Output(UInt(8.W)))
port := delayed + in
port
}
}
(Scastie: https://scastie.scala-lang.org/hZnW0zubQI6eGatznHqZbA)
Based on the previous example, one might expect the name of the register to be in_delayed (ie. the prefix is ignored), but actually the name is out_in_delayed. As mentioned, .suggestName is sensitive to the prefix. So what's going on?
It turns out that our naming calls (.suggestName, .autoSeed, and .autoNameRecursively which uses .autoSeed) all will set the prefix used for naming whether or not they change the actual name. This somewhat weird behavior leads to the first example above being somewhat intuitive (the .suggestName appears to only affects the "local" name of the signal) while making the second example pretty inconsistent with it although also defensible--since the reg is defined within and does not escape a prefixed scope, it gets the prefix!
While the behavior above is okay if a little hard to describe exactly why things happen the way they do, this behavior can cause some pretty bananas behavior:
class Example extends Module {
val in = IO(Input(UInt(8.W)))
val out = {
val delayed = RegNext(in).suggestName("in_delayed")
val port = {
val x = IO(Output(UInt(8.W)))
delayed.suggestName("ignored")
x := delayed + in
x
}
port
}
}
(Scastie: https://scastie.scala-lang.org/8Ci31sNyTQ21m3T4yjuxKg)
After my completely clear and intuitive explanation above, the resulting name that the register gets should be very intuitive right? We all certainly expected the name out_port_in_delayed right? I grant this is somewhat convoluted, but it is possible to hit this accidentally, especially over multiple function calls where one might not realize .suggestName is being called.
I'm not sure about the best fix here because having the naming APIs clear the prefix if the named value is returned from the naming scope actually makes some sense. Perhaps the way to fix it while keeping naming intuitive is so migrate from the mutable .suggsetName-style user API for controlling names to a more immutable withName(<name>)(block) style.
This bug (or feature?) goes back to the original PR adding the compiler plugin: https://github.com/chipsalliance/chisel3/pull/1448
Type of issue: bug report
Impact: API modification
Development Phase: request | proposal
What is the use case for changing the behavior?
More intuitive naming behavior
Isn't the "bananas" case just a bug in suggestName, where we check if the name has already been suggested, but only guard part of the function with that? Shouldn't suggestName just return immediately if it's already been called before? Currently it's still doing the prefix stuff even if suggested_seed was already set. That seems like the bug/quick fix to me... I don't understand the use case for setting the prefix with suggestName if you don't actually take the suggestion.
