Fix mem infer

[donggyukim]

• Fixes RemoveCHIRRTL to infer read port enables correctly
• Adds Readwrite port inference for backward compatibility. This pass successfully infer readwrite ports of srams in rocket-chip.

[aswaterman]

So the strategy is to infer RW ports based upon mutual exclusivity of the conditions? That's fine with me, but we should probably get consensus.

[donggyukim]

Yes, that's the same strategy as in Chisel2.

[azidar]

My opinion is that there should be a way to explicitly call out a readwrite port in Chisel which will generate a readwrite port in Firrtl. In addition, this pass should be optional, not required in the mainline Firrtl compiler.

[donggyukim]

Why don't we make it optional when the explicit interface is available in Chisel?

[azidar]

What if you actually don't want a readwrite port, and instead want separate read and write ports? There is no way to stop the inferencing from happening.

[donggyukim]

I think designers want it to happen in general. Readwrite ports are inferred only for srams, which is beneficial.(Rocket-chip assumes this) I believe this should be done by default, but an option can be provided to turn it off.

[jackkoenig]

This check is impossibly brittle, at the bare minimum we will probably change the number of skips emitted let alone the names of generated nodes.

A better way to check would be to look at the actual DefMemory in the Firrtl tree and check that it has the parameters desired.

[jackkoenig]

Could this type of inference not be reserved for just "infer" memory ports such that if a Chisel user wants this behavior they use the standard Mem.apply (which emits infer)?, If the user wants to guarantee read and write ports they could use Mem.read and Mem.write respectively which specify the directions.

[donggyukim]

In this case this should be done before remove CHIRRTL, but it's pretty hard to infer readwrite ports until expand whens. Maybe, FIRRTL can be fixed to support "infer"?

[azidar]

I think its fine if the readwrite port inference is aggressive, I just think there should be an option (like inline instances) to turn it off/on.

[ben-k]

I agree with @azidar - I think many users instantiating memories will want to explicitly specify the number of ports. Inference can take over only if the user does not give this option.

[aswaterman]

I'm also fine with adding explicit RW ports to Chisel, so that there is no
inference at all. Just need to agree on the API.

On Wednesday, July 27, 2016, Ben Keller notifications@github.com wrote:

I agree with @azidar https://github.com/azidar - I think many users
instantiating memories will want to explicitly specify the number of ports.
Inference can take over only if the user does not give this option.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#203 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA-7wtF9faAH-wDh82WhJCKUf6my-39Dks5qZ7ICgaJpZM4JVnXS
.

[donggyukim]

This shouldn't be turned on by default if there's an explicit API in Chisel.

[azidar]

Agreed. I vote for explicit API in Chisel and optional Firrtl pass to infer readwrite ports.

[azidar]

Looks like you need to rebase onto the new master and try compiling - Begin has been replaced with Block, causing Travis to fail.

[donggyukim]

I make this optional, turned off by default.

[azidar]

Update usage string.

[azidar]

Add comment explaining cases, something like:
a complement of not(a)
a complement of eq(a,0)

[azidar]

Looks good, address my comments and I'll merge.

[shunshou]

Quick question: Is there any reason in particular you use NodeKind instead of MemKind for line 131 of InferReadWrite.scala?

val rw_exp = WSubField(WRef(mem.name, ut, NodeKind(), ug), rw, ut, ug)

For consistency, it seems like a reference to a mem should use MemKind as per something like
val rw_exp = WSubField(WRef(mem.name, ut, MemKind(allPorts.toSeq), ug), rw, ut, ug)

Or rather, is there some reason not to have MemKind? b/c I was doing pattern matching on MemKind's so that I could swap references to Mem out with blackboxes... But once I enabled the InferReadWrite pass, my pass was breaking b/c it couldn't find MemKind's anymore...

Thanks!

[donggyukim]

rw_exp is not memory, so I think it can't be MemKind. Instead, why don't you just search DefMemory?

[shunshou]

Ok -- I was getting some weird cannot find key error without doing that when I just plugged in InferReadWritePass to MiddleFirrtlToLowFirrtl. Now I'm using --inferRW , and I think the additional passes in InferReadWrite.execute clear up that problem. Sorry!

I still don't understand though -- rx_exp is a subfield in mem, but isn't mem (what WRef is referring to) a MemKind even though the subfield is a node?

[shunshou]

In general, what should be the set of passes that should be re-run following each custom FIRRTL pass? CheckInitialization, ResolveKinds, InferTypes, ResolveGenders? Should passes work standalone or should they always be grouped with a set of CheckBlah-type passes?

[donggyukim]

Add InferReadWrite(Transform) before MiddleFirrtlToLowFirrtl(Refer to LoweringCompilers.scala for example), which contains necessary passes for inference.

[azidar]

Generally, all of the checks in the ResolveAndCheck() transform will fix all the working IR nodes (WRef etc.) and check the validity of the circuit. To be extra safe, you can just run that transform after your custom pass.

The standalone unit is Transform - passes are just the unit of modularity within the transform. Thus, the state of the circuit is expected to be consistent between transforms. If you are writing a custom pass, I would highly recommend writing a Transform instead. This guarantees your input is well formed, and if you run all the passes in ResolveAndCheck, then you guarantee your output is also well formed.

[shunshou]

Ok, just for clarification, I'm pretty sure that WRef should be using MemoryKind, because the reference is to an instance of the memory, and WSubField is referring to to a subfield of the memory instance. And the reason why the Transform flavor works is that it runs a ResolveKind which, I would assume fixes the problem (at least based off of what Adam described earlier to be the function of ResolveGender -- I haven't stared at the code / tried confirming any of this).

I need the kind to be MemKind b/c I'm trying to swap out all WRefs to the memory instance, and WRef doesn't contain a DefMemory -- it just houses a string with the name of the memory instance, kind, etc., so I need to search for Kind. However, now that I think about it, maybe running ResolveKind would mean that I don't need to manually update WRefs. The issue was that in Middle -> Low FIRRTL, LowerTypes was run before ResolveKinds and LowerTypes does some memory specific manipulations by looking for MemoryKinds. After swapping out the DefMemory for a DefModule, it was failing to find certain keys b/c the WRefs were still using MemKind.

Not sure if the above makes any sense.

On that note, while you should be writing Transforms and not passes, shouldn't you strive to make passes (that have possible mistakes that could then be fixed via these ResolveX passes) as correct as possible from the get-go?

Because adding nodes with all of the right expressions, etc. requires a lot of utility functions / many code updates, I can see where the FIRRTL pass writer might want to be lazy and leave everything as UnknownGender, default, etc. etc., and just leave the followup passes to correct things that are incompletely/incorrectly defined, but then you should enforce (and not highly recommend) that one can't add passes and can only add Transforms to FIRRTL.

Also, have you guys thought about the point in which calling the same Resolve/Check passes over and over again (for pass safety reasons) for each custom pass becomes unreasonable?

[donggyukim]

MemKind requires port names, but ports are not final in this path. We can add MemKind with an empty list or the current ports if you think this is better, but eventually kinds should be resolved later. If it's not inferred correctly by the following passes in the transform, let me know.

We generally want to make types, kinds, and genders unknown and correctly inferred by resolve/check passes. Of course, you can correctly specify them if you 100% make sure what are they, but unfortunately I can't.

[shunshou]

I believe it should be inferred correctly by the following passes in the transform, since the errors go away after I use the transform, but then I think it's critically important to emphasize to pass/transform writers that they should not be using passes standalone (and being able to specify unknown type/kind until a separate resolve* pass is run makes life a lot easier).

[donggyukim]

Yes, I agree.