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

feat: Memory macro #10

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

feat: Memory macro #10

wants to merge 4 commits into from

Conversation

winston-h-zhang
Copy link
Contributor

This PR implements a defmem macro to automatically generate memory program. We have 3 types of memory:

  • full: These are fully specified memories like cons, fun.
  • digest-only: These are memories that only support digests, like builtin.
  • immediate: These are for immediate types like num.

Depends on #9, please merge that first.

Comment on lines +393 to +394
(relation (hash4 element wide wide wide wide)) ; (tag a b c d)
(relation (unhash4 element wide)) ; (tag digest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this change?


;; TODO: Maybe it would be better for this to belong here? I've copied this over to evaluation.lisp for now.
#+nil
(defmacro defmem (prog-name superclasses config &body arg-specs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever we decide, let's have it live in only one place rather than duplicate and comment.

Comment on lines +72 to +73
(defun concat-sym (root suf)
(intern (format nil "~A~A" root suf)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use symbolconc

;; for example `allocation::ingress`, and this interferes with execution.
;; There should definitely be a way to prevent this from happening in the first place,
;; but I haven't been able to get anything to work.
;;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not totally following the issue, but it's not worth getting into the weeds on now, so let's just live with this comment.

Copy link
Contributor

@porcuquine porcuquine 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 pretty good. I didn't thoroughly check the actual macro, but it seems reasonable and tests are passing. Please just add a test of the macroexpansion for the most complicated case (which I think is cons-mem?) — so we have a clear record of the intended behavior.

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.

2 participants