Single Port Memories - Githubissues

calyxir / calyx

Intermediate Language (IL) for Hardware Accelerator Generators

https://calyxir.org

MIT License

453 stars 45 forks source link

Single Port Memories #1610

Closed andrewb1999 closed 3 months ago

andrewb1999 commented 10 months ago

This is definitely still a work in progress, mostly need to fix all the frontend test cases. Will need to coordinate with all the frontends too.

rachitnigam commented 10 months ago

@EclecticGriffin is this a hard change to make in the interpreter?

EclecticGriffin commented 10 months ago

This shouldn't be too hard to implement in the interpreter, the only weirdness being that some adjustments will be needed in the SeqMem state-machine since there's no longer an explicit error state if I'm understanding things correctly. I can handle that today probably if desired

EclecticGriffin commented 10 months ago

Okay took care of the interpreter side, though I can't be sure there aren't any obvious mistakes until whatever is upsetting the parser is fixed

andrewb1999 commented 10 months ago

Not exactly sure why the parser is failing here (and I don't have enough experience with the parser to debug this). Any ideas @rachitnigam ?

EclecticGriffin commented 10 months ago

my guess is that it has to do with the static annotation? I'm not clear on whether or not the old style is fully deprecated yet, but the parser does treat static as a keyword and so might be mis-parsing because of that? Here's a stupid idea, try reordering the annotations so that the static attribute is never the first one. I think the repeated attribute parsing might not explode then?

rachitnigam commented 10 months ago

Weird error!

rachitnigam commented 10 months ago

Ah, missing comma after content_en is the problem! Normally, if you forget commas, error locations become really inscrutible!

rachitnigam commented 10 months ago

Should be fixed now!

rachitnigam commented 10 months ago

Remaining failures seem legitimate. We also need to update Dahlia to use the right ports. Maybe we can recruit @calebmkim's help on this

calebmkim commented 10 months ago

Yeah I can try to implement a fix for Dahia... should be pretty simple.

calebmkim commented 10 months ago

One thing I'm noticing is that we should change the @write_together annotations. Right now write_data and write_en are required to be driven together, but we shouldn't necessarily require that, since if we want to read, we would drive write_en to 0, and not set write_data.

I think we probably want the following ports driven together: content_en, write_en, and all the address ports. Any time you set content_en to high, you should specify a) whether to read/write and b) which memory addresses to access.

rachitnigam commented 5 months ago

This is getting stale. @andrewb1999 what are the specific blockers to getting this merged?

andrewb1999 commented 5 months ago

I was mostly just waiting for front end changes to make sure this won't break stuff, but yeah this is getting stale now. I'm happy to fix the staleness of it but before I do that we should make sure we are ready to merge and fix any issues that may arise with frontends.