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

Piezo, PIFOTree: static option! #1783

Merged
merged 17 commits into from
Nov 20, 2023
Merged

Piezo, PIFOTree: static option! #1783

merged 17 commits into from
Nov 20, 2023

Conversation

anshumanmohan
Copy link
Contributor

@anshumanmohan anshumanmohan commented Nov 19, 2023

We now have a variant of the PIFOtree-controller-dataplane setup (hereby dubbed "SDN", somewhat grandly) where the stats component is static.

I have made small improvements to the eDSL library, adding static variants as needed. The goal was to continue to do everything through builder.py, never needing to directly access the Calyx AST. I've been successful in that. Other programs could eventually use my new additions for cleaner code.

Overall this looks successful to me: the static version is producing the same output as the dynamic version upon being fed 100 commands.

@anshumanmohan
Copy link
Contributor Author

@rachitnigam just tagging you in case you see an easy way to reengineer the runt clumsiness.

  1. Re: -d well-formed:
    I know how to pass a bespoke flag to a "glob" of files, frequently a directory. But is there any way, within a directory (in this case [calyx-py] correctness), to pass a special Calyx flag to one file?

  2. Re: --static:
    Maybe this was a poor idea, and I should just have a separate .py, .data, .expect trio?

add_cell.left = reg.out
add_cell.right = const(width, val)
reg.write_en = 1
reg.in_ = add_cell.out
Copy link
Contributor Author

@anshumanmohan anshumanmohan Nov 19, 2023

Choose a reason for hiding this comment

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

It would have been nice to have reused the existing incr with a static flag as an argument. I got a good way through it, but then the issue is that the dynamic version needs a line incr_group.done = ... but the static version does not.

I'd usually do that with the Python idiom of X if b else Y, but that doesn't tango super well with the eDSL's own with foo as bar: baz idiom.

Copy link
Contributor Author

@anshumanmohan anshumanmohan Nov 19, 2023

Choose a reason for hiding this comment

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

Just paging @sampsyo in case he knows a neat way! Like basically is there some Y such that I could do:

with foo as bar:
  ...
  ...
  X if not static else Y

and the Y would be a no-op, ignored when generating Calyx code?

If I had a nice way of doing this, I could go through and do it for lots of helpers in the eDSL!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the sync chat; it worked like a charm!

@rachitnigam
Copy link
Contributor

Regarding the runt question: the easiest way to do this is define a separate test suite which only captures the glob of files that you want and passing the extra flag to it. Does that make sense?


with stats.continuous:
stats.this().count_0 = count_0_sto.out
stats.this().count_1 = count_1_sto.out
eq_cell.left = flow
Copy link
Contributor Author

@anshumanmohan anshumanmohan Nov 19, 2023

Choose a reason for hiding this comment

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

For now I have just written, desugared, the code that I needed. It is easy to imagine a little sugar like:
static_if_while(CellAndGroup, IfBody, ElseBody), that:

  1. Snaps apart the CellAndGroup into Cell and Group.
  2. Puts all of Group's assignments into continuous assignment land.
  3. Finally turns itself into static_if(Cell.out, IfBody, ElseBody)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now #1785

@anshumanmohan
Copy link
Contributor Author

@rachitnigam yes I see how this solves my -d flag issue!

Any thoughts about the --static issue? Should I just bite the bullet and make a separate trio of files for static PIFO trees? And then the bespoke glob would hunt down that new static trio? Cuz right now, the runt command that is running on branch push is just testing the dynamic version! Runt doesn't know anything about this weird --static flag.

@rachitnigam
Copy link
Contributor

Sorry, what does the --static flag do? If its supposed to generate the same output files but have a different implementation running and you want to do differential testing, you can use the expect_dir field in the test suite configuration to specify the location of the output file.

@anshumanmohan
Copy link
Contributor Author

Thanks Rachit! Yes so the --static flag changes out the guts of the stats component to be static, and also changes the one invoke of the stats component into a static invoke.

For now I have just gone with a new trio of files (sdn.{py, data, expect}), living in their own directory (calyx-py/test/correctness/static/). This was the easiest way to define a glob for runt. Differential testing against the dynamic version is exactly the right idea, but I'm running into a silly engineering bugaboo that we can leave for post-deadline maybe?

@anshumanmohan
Copy link
Contributor Author

I have updated the frontmatter of the PR to reflect the latest state of affairs. I think this is ready for review.

@anshumanmohan anshumanmohan enabled auto-merge (squash) November 20, 2023 14:47
@anshumanmohan anshumanmohan merged commit 666d06e into master Nov 20, 2023
5 checks passed
@anshumanmohan anshumanmohan deleted the static-pifotree.py branch November 20, 2023 14:59
rachitnigam pushed a commit that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants