Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

feat: add comb dialect #949

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

feat: add comb dialect #949

wants to merge 31 commits into from

Conversation

luisacicolini
Copy link
Contributor

@luisacicolini luisacicolini commented Jan 7, 2025 •
edited
Loading

This PR adds the comb dialect.
truthTable operation and wildcard/cases SV semantics are missing, as they will require more infrastructure (and are not that used AFAIU)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Alive Statistics: 90 / 93 (3 failed)

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

Alive Statistics: 90 / 93 (3 failed)

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

Alive Statistics: 90 / 93 (3 failed)

alexkeizer
alexkeizer reviewed Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

I have some comments about the way you model replicate

@alexkeizer
Copy link
Collaborator

To make, say, add, variadic you could add another parameter to the operation, as in:

| add (w : Nat) (arity : Nat)

Which you then pick up in the signature to specify we want arity operands

| add w arity => List.replicate arity (Ty.bv w)

(You might want to specify arity+1 instead, so you don't have to deal with the corner case of having 0 arguments).

For the operation semantics, you could fold over the list of argument using the current binary implementations

@alexkeizer
Copy link
Collaborator

alexkeizer commented Jan 9, 2025 •
edited
Loading

more in general management of bitvectors' width: in the semantics I currently treat it as an additional argument of the operations (when the bitvectors involved need to have the same width), but I'm not sure this is the right approach

This is exactly what we do in LLVM dialect, and so far it's worked out great there, so it feels like the way to go here also!

@alexkeizer
Copy link
Collaborator

I'm not sure quite what you mean by sv behaviour or truth table operations, but feel free to ask if you have more specific questions about them!

@luisacicolini
Copy link
Contributor Author

Thanks for the comments @alexkeizer! I'll start to fix the variadic implementation and the replicate.

sv is systemverilog-specific behavior, whose semantics requires a more complex model (I am not even sure we want to model that quite yet: you can find it in the wildcard and case in/equality sections here).

alexkeizer
alexkeizer reviewed Jan 21, 2025
View reviewed changes
@luisacicolini luisacicolini marked this pull request as ready for review February 7, 2025 15:29
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2025

Alive Statistics: 90 / 93 (3 failed)

alexkeizer
alexkeizer reviewed Feb 11, 2025
View reviewed changes
ymherklotz
ymherklotz approved these changes Feb 21, 2025
View reviewed changes
Copy link
Collaborator

@ymherklotz ymherklotz left a comment

Choose a reason for hiding this comment

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

This looks good to me :) Having n replicate functions is probably the right way to do it too.

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@luisacicolini
Copy link
Contributor Author

@alexkeizer @ymherklotz thanks for the review. I've added the enumfor the icmp op, will add one more example and then I believe we should be finally good to go :)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

@github-actions GitHub Actions
Copy link

Alive Statistics: 90 / 93 (3 failed)

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@alexkeizer alexkeizer alexkeizer left review comments

@ymherklotz ymherklotz ymherklotz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luisacicolini @alexkeizer @ymherklotz