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

Add ability to parse empty control with attributes #1266

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Conversation

rachitnigam
Copy link
Contributor

Okay, this is an admittedly weird thing to do but: this PR adds the ability to parse empty control blocks when they have attributes.

The Good

For example, this can be parsed now:

control {
  @sync; // <-- note the semicolon
}

The reason is that it gives us to experiment with "proto-keywords" like more easily. We currently have the philosophy of trying features out using attributes before we turn them into full blown keywords.

For example, the @sync keyword is an example. The current semantics is that for a program like this:

control {
  @sync A; B;
}

The synchronization happens before A executes. This means that sometimes we have to write programs like this:

control {
  if lt.out {
    @sync no_op;
  } else {
    @sync A; B;
  }
}

Where the no_op group doesn't do anything. With this change, we can write:

control {
  if lt.out {
    @sync;
  } else {
    @sync; A; B;
  }
}

This also has the pleasant outcome of making the meaning of @sync clearer---the synchronization occurs before the group executes. We also don't need to have special handling for invoke.

The Bad

The bad thing is that this is an empty which means that it shouldn't really have any semantics and should be erasable? This would make it important for passes to keep empty control programs around when they have an attribute attached.

On the other hand, imagine the possibilities–a @log keyword for print out values? @delay to insert fake delays between par threads to test their delay insensitivity?

@paili0628
Copy link
Contributor

I think this is a great thing as it certainly makes the semantics of sync clearer. Let me try to change the sync test cases to see if this gets compiled properly to barriers

@EclecticGriffin
Copy link
Collaborator

hmm, I can kinda see this for sync but I have some misgivings. On the whole the idea of using empty groups with special attributes seems to me a means of hacking in a set of auxiliary control psuedo-commands (pragmas?) that influence compilation but don't correspond to group-type compilation flows. And if that's what we want, it might be better to let those types of commands stand alone.

lmk if this makes any sense. This is just my initial reaction so I am open to being persuaded.

@rachitnigam
Copy link
Contributor Author

@EclecticGriffin absolutely! Thinking of these as pseudo commands is pretty much what I was going for. I agree that we would eventually want to turn all of these into real control operators to do it “the right way”

Having this backdoor is more a question of making it easier to experiment with things quickly. Adding a new command is an infrastructure-wide change while adding a new attribute is trivial.

In fact, it also makes it easier for someone outside of the core team to play with new control like operators because all they have to do is add the attribute and write a pass to interpret it

@EclecticGriffin
Copy link
Collaborator

makes sense to me!

@sampsyo
Copy link
Contributor

sampsyo commented Nov 14, 2022

Yeah, I too agree that this is a strange but creative solution to the problem! Seems like a good idea to run with it. In all fairness, attributes were already not "semantics-free"… especially in the case of @sync, your program's correctness really relies on the synchronization actually happening. So this is not much of a slide down the slippery slope, IMO, and it does open the door to interesting non-built-in control statements.

@rachitnigam rachitnigam enabled auto-merge (squash) November 15, 2022 21:51
@rachitnigam rachitnigam merged commit d04940d into master Nov 15, 2022
@rachitnigam rachitnigam deleted the empty-parsing branch November 15, 2022 22:12
rachitnigam added a commit that referenced this pull request Nov 15, 2022
* Reduce size of Error

* Box span in ir::Id

* Make Span unclonable and behind Rc

* Add ability to parse empty control with attributes (#1266)

* Add autoupdate workflow (#1257)

* Add autoupdate workflow

* disable requirement on eval

* autoupdate separate out workflow
janpaulpl added a commit that referenced this pull request Nov 29, 2022
* add CompileInvoke to no-opt pipeline (#1206)

* allow passing tcl file (#1207)

* [Interpreter] Signed fix (#1208)

* fix the parse bug

* fix black invocation

* Reorder logical and expressions. (#1209)

* Fix Papercut For Multiple Go-Done Pairs (#1210)

* Do not group2invoke components with multiple go/done pairs (#1211)

* Update Contributor List (#1212)

* fixes LeNet (#1213)

* Fix Dockerfile not working with runt cocotb tests (#1216)

* Cocotb-bus is now built from github repo

LANG env variable is also set explicitly to be utf-8

* Fix calyx branch and reduce apt-get calls

Co-authored-by: Nathaniel Navarro <nrn25@cornell.edu>

* small changes to bounded sharing (#1214)

* Vivado Maps seq-mems to Urams (#1219)

* vivado maps to urams

* small change

Co-authored-by: Caleb Min Sup Kim <cmk265@havarti.cs.cornell.edu>

* facilitate ref declaration (#1218)

* facilitate ref declaration

* check that a cell is not both external and passed in by reference

* fix types

* Any Base Exponentiation  (#1226)

* added any base

* got rid of muck

* got rid of code garbage

* rewrote tests

* Adding reserved words (#1236)

Discussed in #1233

* [Interpreter] turn off group assignments the same cycle done becomes high (#1237)

* add alias to avoid confusion

* using boxed error instead

* bunch of clippy lints

* bug fixing

* Print out X-values in dat output (#1242)

* Generate X-values in the final DAT file

* format frontend code

* High-level "builder" (kind of an embedded DSL?) in the Python library for easy program generation (#1229)

* PoC Python AST builder library

* Operator overloading for guard expressions

* Absurd contextual assignment syntax

* Bind group names in context statements

* Ridiculous avoidance of Python builtins

* Support Python integer literals

* Support width inference for `done` holes

* Naming & docs

* Slight context access reorg

* Simplify the example a tiny bit

* Flip assignment ordering

For the modern AST library, which has sensibly reversed these things.

* Mild Python cleanup

* New attempt at conditional assignments

* Type cleanliness

* More docs suggested by @cgyurgyik

* Avoid pyramid. (#1243)

* Small LiveRangeAnalysis Change (#1222)

* small lra change

* documentation change

* clarified code + docs

* added clarifying comments

* save_mem flag to allow `dense` operation to save memory  (#1230)

* allow dense to save memory

* addded clarifying comment

* made relay_visitor.py runnable as script

* simplified save_mem flag and added documentation

* added lrn, cleaned up gen_exp (#1231)

* added lrn, cleaned up gen_exp

* updated docs

* small doc changes

* super small change (#1247)

* Check that there are no ref cells before compiling invokes (#1248)

* Disable static timing tests (#1252)

* `group2invoke` should not convert groups with guarded assignments to go/done ports (#1251)

* `group2invoke` should not convert groups with guarded assignments to go/done ports

* comment about wait group

* Comment about the wait restore group

* synchronize before starting new iteration

* remove redundant sync test

* skip sync-while test

* Preserve attributes in group2invoke (#1256)

* `group2invoke` should not convert groups with guarded assignments to go/done ports

* comment about wait group

* Comment about the wait restore group

* synchronize before starting new iteration

* Fix #1253

* fmt

* Fix std_fp_mult_pipe Primitive (#1246)

* Fix std_fp_mult_pipe primitive

* Maybe enable real pipelined multiply

* update exp tests

* systolic and ntt test updates

Co-authored-by: Rachit Nigam <rachit.nigam12@gmail.com>

* separate out compiler tests (#1258)

* Check read and write specs in combinational groups (#1259)

* clippy warnings

* Read write specs checked in comb groups

* add test

* Don't use `then_some` which is not available in older version

* Bump minimatch from 3.0.4 to 3.1.2 in /web (#1263)

Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.0.4 to 3.1.2.
- [Release notes](https://github.com/isaacs/minimatch/releases)
- [Commits](isaacs/minimatch@v3.0.4...v3.1.2)

---
updated-dependencies:
- dependency-name: minimatch
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix #1239 (#1267)

* Add ability to parse empty control with attributes (#1266)

* Add autoupdate workflow (#1257)

* Add autoupdate workflow

* disable requirement on eval

* autoupdate separate out workflow

* Revert "Add autoupdate workflow (#1257)"

This reverts commit e3c1412.

* limit concurrency in actions (#1271)

* Reduce struct sizes (#1269)

* Reduce size of Error

* Box span in ir::Id

* Make Span unclonable and behind Rc

* Add ability to parse empty control with attributes (#1266)

* Add autoupdate workflow (#1257)

* Add autoupdate workflow

* disable requirement on eval

* autoupdate separate out workflow

* Fix #1244 (#1268)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix #1277 (#1279)

* Add timing specified timing information to latency_data

* Fix #1277

* Generate program without buffering (#1280)

* vscode nonsense

* print to output stream directly instead of buffering

* Generate body without buffering

* flip

* update tests

* Upgrade dependencies

* Revert "Upgrade dependencies"

This reverts commit b807bf5.

* Upgrade dependencies (#1281)

* Upgrade dependencies

* rust upd

* Log parsing time

* CDF Generator  (#1275)

* started on cdf-gen

* more progress

* cdf generator

* formatting

* updated cdf formulation to be more accurate

* cleaned code changed way we count sharing

* clippy

* Make builder work with python 3.9

* add test for builder

* Update Verilator v5.002 (#1289)

* vscode nonsense

* Fix #1260

* update tests

* Use builder for systolic frontend (#1288)

* vscode nonsense

* Port systolic frontend to use builder

* update test

* remove pe def

* cleanup

* upd

* [Interpreter] Misc reorg (#1290)

* commit regression test from earlier

* Do some light reorganization

* fix the tests

* use usize instead of u64

* Ref problem (#1255)

* fixed the issue with ref

* added comments

* made changes to insert port in the start function

* formatting issue

* modified compile-ref and test cases

* format

* change compile-sync to original version

* formatting

* canonicalize

* fix borrow error

* fixed small error

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Rachit Nigam <rachit.nigam12@gmail.com>

* Update documentation (#1291)

* Fill TK in lang-ref

* inlining trick

* document data format

* minor fixes

* Modify sync (#1285)

* change compile-sync

* modified compile-sync so it adds barriers to single group enables and cell invokes

* changed compile-sync and modified passes

* modify tests

* changed example test cases

* added error message, changed name for `wait`, changed markdown

* format

* fix format

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Rachit Nigam <rachit.nigam12@gmail.com>
Co-authored-by: Griffin Berlstein <griffin@berlste.in>
Co-authored-by: Chris Gyurgyik <37983775+cgyurgyik@users.noreply.github.com>
Co-authored-by: Andrew Butt <andrewb1999@gmail.com>
Co-authored-by: calebmkim <55243755+calebmkim@users.noreply.github.com>
Co-authored-by: Nathaniel Navarro <nathanielnrn99@gmail.com>
Co-authored-by: Nathaniel Navarro <nrn25@cornell.edu>
Co-authored-by: Caleb Min Sup Kim <cmk265@havarti.cs.cornell.edu>
Co-authored-by: Susan Garry <sgarry406@gmail.com>
Co-authored-by: Adrian Sampson <adrian@radbox.org>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: paili0628 <92661158+paili0628@users.noreply.github.com>
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.

4 participants