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

[Proposal] Optional embedded metadata in Calyx #994

Closed
EclecticGriffin opened this issue Apr 29, 2022 · 6 comments · Fixed by #996
Closed

[Proposal] Optional embedded metadata in Calyx #994

EclecticGriffin opened this issue Apr 29, 2022 · 6 comments · Fixed by #996

Comments

@EclecticGriffin
Copy link
Collaborator

EclecticGriffin commented Apr 29, 2022

To document some of the things we've been discussing synchronously. The current metadata system/format works with pos attributes attached to leaf nodes in the Calyx control tree (invokes and enables) and a separate metadata file which defines a map between these position tags and the source position to emit when active.

The mapping file being separate from the Calyx code it is attached to creates a bit of a "linking" problem as it requires our toolchains (and really the end user) keep track of where the file is and what Calyx program it belongs to. The advantage of this is that the main Calyx parser does not need to be aware of the metadata and if a program is not being given to the debugger there is minimal overhead for the position attributes.

On the whole after talking to y'all and @cgyurgyik, I am generally of the opinion at this point that it would be better overall to have some in IR notion of an optional metadata table/blob as part of the file itself.

There are some disadvantages to this in that it requires the Calyx parser to be aware of the metadata format and places it on the main path with respect to maintainability. This also introduces a "backedge" type of dependency between Cider and the compiler which is potentially undesireable.

On the other hand, this makes the metadata far easier to use on the toolchain (no need to track separate files) and making it an optional component should hopefully reduce some of the overhead from having it in the first place. This also gives us a good site to think about further extensions to the metadata in tandem with language changes and gives us the opportunity to sanitize and otherwise operate on the metadata through program transformations though I don't know whether we would actually want to do that.


As a concrete syntax proposal, we could add an optional block at the end of the file which looks like:

METADATA {
...
}

where the internals of the block are the current contents of the (separate) metadata file.

I think this is no harder for front-ends to generate as it requires the same information but frees them of needing to open/write a separate file.

@cgyurgyik
Copy link
Collaborator

+1

I think the the approach mentioned above is already better than what we currently have. Another (future) suggestion would be to embed a Calyx program in some top-level module, similar to CIRCT. So that this can also use the attribute schema. For example,

module {

component Foo() -> () {
  ...
  control { @pos(0) enable A; }
}
...
} @metadata(0: Line100:Col3, ...)

@sampsyo
Copy link
Contributor

sampsyo commented Apr 29, 2022

Yeah, this sounds great if it simplifies the weird file-on-the-side logistics we're currently dealing with! IMO a hacky approach where METADATA { ... } just contains a totally opaque string blob is a good way to get started and we could refine it down the line.

To expand on what @cgyurgyik said, the LLVM way to do this would be that the metadata is structured, i.e., it is implemented as an arbitrary data structure of sorts. That would look notionally like this:

component foo() -> () {
  control { @pos=0 a; }
}

@metadata 0 = "file xxx at line yyy and column zzz";

Glancing over the LLVM LangRef's metadata section could be instructive. Anyway, just getting this out of the way to say it's what a fully-over-engineered version of this would look like, and that a flat blob seems fine for now.

I didn't quite understand this:

This also introduces a "backedge" type of dependency between Cider and the compiler which is potentially undesireable.

I don't quite see how the compiler gets involved here TBH?

@EclecticGriffin
Copy link
Collaborator Author

I don't quite see how the compiler gets involved here TBH?

Sorry I meant the language parser, which I suppose is distinct from the compiler itself

@rachitnigam
Copy link
Contributor

+1 for Metadata hack. We can bikeshed on the syntax once the PR is up but I think parsing it as an opaque string and making it available to the context is the good thing to do.

@cgyurgyik
Copy link
Collaborator

Glancing over the LLVM LangRef's metadata section could be instructive. Anyway, just getting this out of the way to say it's what a fully-over-engineered version of this would look like, and that a flat blob seems fine for now.

I was more so thinking of just abusing MLIR attributes, but I think they're both capable of accomplishing the task :-).

@cgyurgyik
Copy link
Collaborator

Worked up a simple prototype for this in llvm/circt#2959.

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 a pull request may close this issue.

4 participants