peek.litValue is confusing

[schoeberl]

I think we have a confusing naming here with ChielsTest and the notion of a literal. I understand that ChiselTest is using Chisel types for peek and poke. However, in a testing environment, we may use Scala land types and we need to convert. A function called litValue is hinting in its name that this is a literal/constant. However, during tests this is not the case. It may return a different value on a peek each clock cycle! So the name lit is misleading. Maybe something like value`` or asBigInt``` is more appropriate.

Luckily ChiselTest is still in alpha mode and we can change the API for a more consistent naming ;-)

Cheers,
Martin

[ekiwi]

Luckily ChiselTest is still in alpha mode and we can change the API for a more consistent naming ;-)

Unfortunately this is a chisel API: https://github.com/chipsalliance/chisel3/blob/c5861176887bfa529277e686df09a42aeceb6cd7/core/src/main/scala/chisel3/Data.scala#L576

However, we could probably add a better name in addition to the old method. I do like asBigInt. @ducky64 has more experience with this end of Chisel.

[ducky64]

So I'm not sure I completely understand what you mean: but what litValue is intended to mean is "if this Data is a literal, return its bitvector value as a BigInt". The returned value of litValue should not change, and since Data is immutable, the same Data object should always always return an equivalent litValue.

However, every time you call peek, it returns a fresh literal Data based on the current value of the wire being peeked.

If you're asking for some way to check if a wire in a circuit is truly const, and return its value in that case (and perhaps crash otherwise?), that functionality doesn't exist. I'm not sure how useful it will be either.

Possibly related: see chisel3 #830 for prior discussion on litValue (and considered alternatives such as litToBigInt, and alternate forms litToBoolean and litToDouble).

[schoeberl]

I understand that the API comes from Chisel and not from the tester. And I think mixing this is the issue. Having a HW constant as a Chisel type is fine. 2.U is a constant or a literal. But when testing the result it is not a constant (even when the result is internally is represented as a Chisel type constant allocated on the heap - this is mixing internal representation with an external view).

Let's rephrase the issue:

def foo(a: Int, b: Int) = a + b

This function will return a different value for each different a and b. Easy. You would never call the result of foo(...) a constant or literal, right?

Maybe I am picky, but a return from a function (peek) is usually not a constant/a literal value. asBigInt for converting it to Scala testing land would be more natural choice.

[ducky64]

I'm still lost.

peek() on a wire (Chisel Data type associated with some hardware, and without a constant value) returns a Chisel literal (a Data type with a constant value, eg 2.U). We return a Chisel literal here, because prior versions (PeekPokeTester) used regular Scala types and that proved limiting (UInt, SInt, Bool, FixedPoint have Scala analogies, but what does it mean to return a Vec? what about a sparse Vec? what about a Bundle?).

I guess my question is ... what do you want to see?
Do you just want a one-step extractor from wire to Scala number? eg, peekToBigInt? Or a better name for litValue? (note that the latter was part of the discussion on chisel3 #820 and was rejected for compatibility reasons - though doesn't mean the discussion can't continue)

[schoeberl]

Maybe this is getting a bit philosophical. What is a literal? for me this is a constant, something that does not change. Maybe not a big thing, but it is a least a stable contract. When a function (foo() or peek()) returns something, than this is not a constant/literal but a value. It is computed by the input values and maybe be some internal state. So peek() returns a value not a constant. It may be different on each invocation. So no constant.

Am I picky here? ;-)

[ducky64]

Ah, I think I see. Currently the object returned by peek() is a literal (and does not change value, and we can do things like litValue on the output), but peek() does not return the same object every time. So while what is returned by peek() is (loosely) a constant, peek() itself is not a constant.

[ekiwi]

I feel like a good summary here would be that (1) litValue makes since from a hardware construction language standpoint but (2) this name is rather unintuative from a testing standpoint.

I wonder if it would be possible to allow UInt/SInt types to implicitly be cast to BigInt. What do you think @ducky64 ?

[ducky64]

My immediate feeling is that the implicit conversion could end up being confusing - I think we've generally had problems with getting non-PML implicits to be robust. It might be worth prototyping and seeing how it works in a few realistic use cases, but I don't have high hopes for it?

But yes, I agree that litValue is pretty unintuitive in this case. The hard part is coming up with something better. I think seeing a couple of use cases where this is causing issues could be helpful in formulating alternatives?

[schoeberl]

peek() returns a value and is not even a pure function as in functional languages as the return value depends on the state of the circuit and not on any parameter of the function peek(). Yes, the returned value is an immutable Chisel type, but not a literal. 123 and "abc" are literals, val c = "abc", c is a constant not a literal (in C/Java you would use the term const). If you have the following function:

def foo(a: String, b: String) = a + b

the return value is a String value, not a literal or constant. Although strings in the JVM are internally constants and immutable.

So peek() returns a value as a Chisel type and to use it in Scala land we need to convert it, e.g., to a BigInt. So I argue to call this function asBigInt(). Maybe we would also like things such as asBoolean or asInt.

[schoeberl]

Maybe we should start within Chisel to change from asSomeThing to toSomeThing, as this is the Scala way to convert between types.

[ducky64]

So in Chisel we call things like 2.U and true.B literals (sure, strictly speaking they're not Scala literals, but they're close enough in terms of the Chisel HDL). Yes, the name is perhaps overloaded with a PL term that has a separate meaning.

In terms of renaming: feel free to propose as a chisel3 issue. But a main concern there is to minimize maintenance burden on legacy code, so I wouldn't be optimistic that a stylistic renaming without a large usability benefit will go through.

We also do things like litValue and litToDouble to indicate that those methods can't be called on any Data (what does it mean to do asDouble on a wire?).

In any case, unless you have a concrete proposal for a chiseltest change, this seems more like a chisel3 issue that should be discussed either as a chisel3 issue or during a dev meeting?

[ekiwi]

In terms of renaming: feel free to propose as a chisel3 issue. But a main concern there is to minimize maintenance burden on legacy code, so I wouldn't be optimistic that a stylistic renaming without a large usability benefit will go through.

I feel like just adding an alias might be acceptable.

[ducky64]

I'm generally not a fan of having multiple ways to do the same thing without some kind of deprecation automation (even if the old API never gets removed, and individual deprecations can be suppressed), otherwise we just end up with a confusing mix of old APIs that people never migrate off of because they don't know, and new people not being clear on which option to use.

In any case, this seems like a chisel3 / dev meeting discussion.

[schoeberl]

OK, I agree that changing the name of a function that is widely used is not really an option (anymore). And I agree with @ducky64 that it is not a good idea to have two names for the same function.

I agree that 2.U and true.B shall be called literals. Similar to 1 or "abc" in Scala. But peek() does not return something that I would call a literal (even when it implemented internally by using Chisel-type objects that are also used to represent HW literals).

Yes, let us pick this up on a Monday discusssion.

[carlosedp]

Maybe even having a simpler alias to peek().litValue like .getLit () to reduce verbosity and use native Scala types?

[ekiwi]

Maybe even having a simpler alias to peek().litValue like .getLit () to reduce verbosity and use native Scala types?

I think something like that might be interesting. It would only work on UInt and SInt, but that might already be enough. Or would you need to it work on other types?

[carlosedp]

I think working on SInt, UInt and Bool is sufficient for most cases (if not all) :)
I can take a look on implementing it and sending a PR... any pointer where I should look at starting?

[jackkoenig]

I'm amenable to making Chisel literals act more like Scala literals (including in operations like ==) such that you could just do .peek() == 5.U or whatever. It is definitely a tradeoff within chisel3 that I discuss in the StackOverflow issue: https://stackoverflow.com/a/68928263/2483329

Obviously this would need some thought though.

[carlosedp]

I have no issue with working with Chisel's own types for hardware generation, also I think it helps on keeping your thinking on "this is generation and that is hardware" but I believe for testing, the conversion to native Scala types would benefit some automation and comparison using validation functions and etc...

I think having a function returning the native value would reduce verbosity too.

[jackkoenig]

The main problem with returning native Scala values is handling of Aggregates. We could return Vector instead of Vec, but Bundles remain a problem, especially if you want to use . syntax on them. We could return a Map, but it'd end up being Map[String, Any] in practice which isn't very good. We could play some heroic tricks with shapeless+dynamic+macros to make it actually type safe, but such tricks don't work with IDEs and won't work in Scala 3 so I would advise against it.

[schoeberl]

fixed by #480