Serializer: only serialize smem RUW if non-default

[ekiwi]

This will make it easier to compile circuits with
older compiler versions.

[mwachs5]

whats the plan to get rid of this in the longer term though?

Sorry can someone list out the steps here to getting to the right final state?

[mwachs5]

default according to firrtl spec or ...?

[ekiwi]

whats the plan to get rid of this in the longer term though?

Sorry can someone list out the steps here to getting to the right final state?

We do not need to do anything. The firrtl parser treats this attribute as optional and will assume the default is Undefined. This has been the case for a long time. Just the serializer used to drop the attribute everytime, even when it wasn;'t the default value.

[jackkoenig]

Can you also add a test?

[ekiwi]

Can you also add a test?

The current tests already cover this mostly. Do you really want a test that ensures that undefined is not emitted?

[ekiwi]

We are already testing that the parser can deal with an omitted undefined and that it will choose the correct default.

[jackkoenig]

Do you really want a test that ensures that undefined is not emitted?

Considering its presence not working with existing releases of firtool is what brought us here, yes 🙂

[mwachs5]

If this is supposed to be optional can someone clarify the firrtl spec that if it is omitted its undefined? My read of the spec does not make me think that this is actually optional:

https://github.com/chipsalliance/firrtl-spec/blob/0190aba6a059c885eff2a01765b3966e81db4533/spec.md#read-under-write-behavior

[ekiwi]

If this is supposed to be optional can someone clarify the firrtl spec that if it is omitted its undefined? My read of the spec does not make me think that this is actually optional:

smem isn't part of the spec afaik. The default is undefined because of the way the firrtl parser is written.

[mwachs5]

what do you mean it's not part of the spec...? Well, can we add it then...?

What is this then:

(* Memory *)
ruw = ( "old" | "new" | "undefined" ) ;
memory = "mem" , id , ":" , [ info ] , newline , indent ,
           "data-type" , "=>" , type , newline ,
           "depth" , "=>" , int , newline ,
           "read-latency" , "=>" , int , newline ,
           "write-latency" , "=>" , int , newline ,
           "read-under-write" , "=>" , ruw , newline ,
           { "reader" , "=>" , id , newline } ,
           { "writer" , "=>" , id , newline } ,
           { "readwriter" , "=>" , id , newline } ,
         dedent ;

[jackkoenig]

CHIRRTL (including smem) has never been part of the FIRRTL spec. It should be, yes.

[jackkoenig]

What is this then:

That is FIRRTL memory which requires eagerly declaring the ports. Chisel (and thus CHIRRTL) mems allow you to declare the ports of a memory separately (and later) than when you declare the memory itself, thus they are separate.

[mwachs5]

So are you saying that if we were to add smem to the firrtl spec, then it would have a default behavior of "undefined" for its read-under-write behavior, different than mem which clearly in the spec does not have a default for read-under-write?

[seldridge]

I'm fine with this approach, too. Thanks for the fix. This'll keep CI passing for now.

[jackkoenig]

So are you saying that if we were to add smem to the firrtl spec, then it would have a default behavior of "undefined" for its read-under-write behavior, different than mem which clearly in the spec does not have a default for read-under-write?

Yes, considering that was the historic behavior it makes sense to have it as the default for serialization and deserialization purposes. There are many differences between smem and mem so I don't think a difference in this behavior really matters.