Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCF-To-Calyx] Fixes Bugs #5573

Merged
merged 4 commits into from
Jul 13, 2023
Merged

[SCF-To-Calyx] Fixes Bugs #5573

merged 4 commits into from
Jul 13, 2023

Conversation

calebmkim
Copy link
Contributor

@calebmkim calebmkim commented Jul 12, 2023

(Same changes as #5557, just using my own fork instead of Andrew's ... sorry for confusing way of doing this).

Two small changes in SCFToCalyx (they're unrelated but both pretty small so I thought both could go in a single PR)

  1. When we see a memref w/o dimensions (e.g., memref<i32>), we should lower it in Calyx to a 1 dimensional memory of size 1, since Calyx memories must have explicitly defined dimensions. Before it was erroring out when trying to export to Calyx, complaining that Calyx doesn't support "0-dimensional" memories.

  2. This is a bit nuanced, but I also changed the way we compile binOps (e.g., multiply, divide, etc.) to be more correct.
    Previously, binOps were compiled into groups as follows:

group perform_binOp {
  binOp.go = 1'd1; 
  // assign to binOp.left and binOp.right 
  reg.write_en = binOp.done; 
  reg.in = binOp.out; 
  perform_binOp[done] = reg.done; 
}

However, this is technically triggering binOp.go for the entire group, which means that it's being triggered even for the cycle that we spend writing into the register. The binOp being triggered for an extra cycle normally doesn't matter, but it does sometimes affect the binOp.out value when we try to read from it later on. Specifically, I was running into tricky bugs when the binOp was a signed division, but this fixed it:

group perform_binOp {
  binOp.go = !binOp.done ? 1'd1; 
  // everything else is the same 
}

Just as an FYI, this way of compiling groups (with the binOp.go = !binOp.done ? 1'd1;) is the standard way to perform a "binOp then write to register" in Calyx. For example, the Dahlia->Calyx compiler does this same behavior

@uenoku uenoku added the Calyx The Calyx dialect label Jul 13, 2023
@calebmkim calebmkim changed the title Fixes Bugs [SCF-To-Calyx] Fixes Bugs Jul 13, 2023
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the same changes as before, which look good to me.

@calebmkim calebmkim merged commit 9402d40 into llvm:main Jul 13, 2023
5 checks passed
@calebmkim
Copy link
Contributor Author

Ok, I'll merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Calyx The Calyx dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants