Make chip-contiguous-subschedule-from-last-instructions non-recursive #693

erichulburd · 2021-01-26T03:02:31Z

Closes #697.

erichulburd

Lisp single-link lists are still not entirely intuitive to me, so want to double check myself on the append and setf/cdr operation below.

Also, any reason to use setq in place of setf here or just generally?

src/addresser/outgoing-schedule.lisp

erichulburd · 2021-01-26T03:07:32Z

src/addresser/outgoing-schedule.lisp

+               (loop for predecessor in predecessors 
+                     do (unless (gethash predecessor seen)
+                          (setf (gethash predecessor seen) t)
+                          (setf queue (append queue (list predecessor)))))


Is this operation efficient memory resource-wise?

Generally, no. It will copy the entire queue, and as a part of that copy, append the element. So it's O(N) in time and space. It's preferred to use an actual queue-like data structure instead of raw Lisp lists.

There are a couple options for FIFO queues in quilc. The first is to use cl-heap. The second is to use the very simple queue data structure in god-table.lisp (see cl-quil.clifford::make-queue).

If you instead just want a stack, then using push and pop on Lisp lists works fine, and is O(1). (I don't think a stack can be used for this BFS though.)

src/addresser/outgoing-schedule.lisp

stylewarning

I noted mostly style and efficiency concerns, which you may incorporate. After that, I'll be happy to check correctness.

stylewarning · 2021-01-26T11:05:05Z

src/addresser/outgoing-schedule.lisp

+               (loop for predecessor in predecessors 
+                     do (unless (gethash predecessor seen)
+                          (setf (gethash predecessor seen) t)
+                          (setf queue (append queue (list predecessor)))))


Generally, no. It will copy the entire queue, and as a part of that copy, append the element. So it's O(N) in time and space. It's preferred to use an actual queue-like data structure instead of raw Lisp lists.

There are a couple options for FIFO queues in quilc. The first is to use cl-heap. The second is to use the very simple queue data structure in god-table.lisp (see cl-quil.clifford::make-queue).

src/addresser/outgoing-schedule.lisp

stylewarning · 2021-01-26T11:06:37Z

src/addresser/outgoing-schedule.lisp

+               (loop for predecessor in predecessors 
+                     do (unless (gethash predecessor seen)
+                          (setf (gethash predecessor seen) t)
+                          (setf queue (append queue (list predecessor)))))


If you instead just want a stack, then using push and pop on Lisp lists works fine, and is O(1). (I don't think a stack can be used for this BFS though.)

src/addresser/outgoing-schedule.lisp

erichulburd

Per discussion with Mark, I also owe a new test case that passes here but will blow up on the previous implementation.

src/addresser/outgoing-schedule.lisp

tests/addresser-tests.lisp

erichulburd · 2021-01-27T19:39:17Z

tests/addresser-tests.lisp

@@ -100,3 +100,26 @@
                   (quil::chip-contiguous-subschedule-from-last-instructions
                    sched (quil::make-qubit-resource 0 2))
                   expected-subschedule))))))
+
+(deftest test-fidelity-addresser-subschedule-on-deep-circuit ()


Can confirm this spec will result in control stack exhaustion with old implementation:

we gotta work on your emacs theme at some point

All I really care about is ENORMOUS FONT. Otherwise, I'm open.

notmgsk · 2021-01-27T20:07:23Z

FYI it looks like chip-schedule-from-carving-point is unused now, and can probably be removed.

src/addresser/outgoing-schedule.lisp

notmgsk · 2021-01-27T20:21:14Z

src/addresser/outgoing-schedule.lisp

-                     ((resource-subsetp (instruction-resources instr)
-                                        resource)
-                      (map nil #'mark-predecessors-seen (gethash instr earlier-instrs)))))
-             (bottoms-up (instr)


This was nice while it lasted 🍻

notmgsk · 2021-01-27T20:21:58Z

tests/addresser-tests.lisp

@@ -100,3 +100,26 @@
                   (quil::chip-contiguous-subschedule-from-last-instructions
                    sched (quil::make-qubit-resource 0 2))
                   expected-subschedule))))))
+
+(deftest test-fidelity-addresser-subschedule-on-deep-circuit ()


we gotta work on your emacs theme at some point

stylewarning

My comments are consistency-in-style nits. Otherwise looks good-to-go.

src/addresser/outgoing-schedule.lisp

erichulburd · 2021-04-13T00:34:28Z

test error

   When attempting to read the slot's value (slot-value), the slot
  COMPILATION-TOLERANCE is missing from the object
  #<CL-QUIL::STATE-PREP-COMPRESSION-TOLERANCE-ERROR {100560AB83}>.
It has a slot CL-QUIL::COMPILATION-TOLERANCE, while CL-QUIL-TESTS::COMPILATION-TOLERANCE is requested.

stylewarning · 2021-04-15T21:14:22Z

test error

   When attempting to read the slot's value (slot-value), the slot
  COMPILATION-TOLERANCE is missing from the object
  #<CL-QUIL::STATE-PREP-COMPRESSION-TOLERANCE-ERROR {100560AB83}>.
It has a slot CL-QUIL::COMPILATION-TOLERANCE, while CL-QUIL-TESTS::COMPILATION-TOLERANCE is requested.

Seems like a simple package issue, somewhere there's a (slot-value foo 'compilation-tolerance) that ought to be (slot-value foo 'cl-quil::compilation-tolerance). (could also be with-slots)

…cursive

notmgsk · 2021-10-23T15:26:50Z

Can this be closed in favour of #717, @erichulburd?

erichulburd requested a review from notmgsk January 26, 2021 03:02

erichulburd requested a review from a team as a code owner January 26, 2021 03:02

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from f73467f to 568dcbe Compare January 26, 2021 03:06

erichulburd commented Jan 26, 2021

View reviewed changes

stylewarning requested changes Jan 26, 2021

View reviewed changes

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from 568dcbe to 0d1084b Compare January 26, 2021 21:50

erichulburd commented Jan 26, 2021

View reviewed changes

src/addresser/outgoing-schedule.lisp Outdated Show resolved Hide resolved

src/addresser/outgoing-schedule.lisp Show resolved Hide resolved

src/addresser/outgoing-schedule.lisp Show resolved Hide resolved

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from 0d1084b to 9c35515 Compare January 26, 2021 21:59

erichulburd requested a review from stylewarning January 27, 2021 01:28

erichulburd commented Jan 27, 2021

View reviewed changes

tests/addresser-tests.lisp Outdated Show resolved Hide resolved

erichulburd commented Jan 27, 2021

View reviewed changes

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from f7ad1f3 to 7331aee Compare January 27, 2021 20:16

notmgsk suggested changes Jan 27, 2021

View reviewed changes

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from 7331aee to afb4812 Compare January 28, 2021 01:38

notmgsk changed the title ~~refactor chip-contiguous-subschedule-from-last-instructions to non-re…~~ Make chip-contiguous-subschedule-from-last-instructions non-ronrecursive Jan 28, 2021

erichulburd requested a review from notmgsk January 28, 2021 18:28

kalzoo changed the title ~~Make chip-contiguous-subschedule-from-last-instructions non-ronrecursive~~ Make chip-contiguous-subschedule-from-last-instructions non-recursive Feb 1, 2021

notmgsk previously approved these changes Apr 1, 2021

View reviewed changes

stylewarning requested changes Apr 1, 2021

View reviewed changes

src/addresser/outgoing-schedule.lisp Outdated Show resolved Hide resolved

src/addresser/outgoing-schedule.lisp Outdated Show resolved Hide resolved

src/addresser/outgoing-schedule.lisp Outdated Show resolved Hide resolved

src/addresser/outgoing-schedule.lisp Outdated Show resolved Hide resolved

erichulburd dismissed notmgsk’s stale review via a298cdf April 12, 2021 23:51

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from afb4812 to a298cdf Compare April 12, 2021 23:51

erichulburd added 2 commits May 10, 2021 14:16

refactor chip-contiguous-subschedule-from-last-instructions to non-re…

498e25f

…cursive

add contiguous subschedule spec for callstack exhaustion

576baef

erichulburd force-pushed the nonrecursive_contiguous_subschedule branch from 4f56b08 to 576baef Compare May 10, 2021 21:17

erichulburd mentioned this pull request Jun 7, 2021

Nonrecursive contiguous subschedule #717

Open

notmgsk closed this Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make chip-contiguous-subschedule-from-last-instructions non-recursive #693

Make chip-contiguous-subschedule-from-last-instructions non-recursive #693

erichulburd commented Jan 26, 2021 •

edited by notmgsk

Loading

erichulburd left a comment

erichulburd Jan 26, 2021

stylewarning Jan 26, 2021

stylewarning Jan 26, 2021

stylewarning left a comment

stylewarning Jan 26, 2021

stylewarning Jan 26, 2021

erichulburd left a comment

erichulburd Jan 27, 2021

notmgsk Jan 27, 2021

erichulburd Jan 28, 2021

notmgsk commented Jan 27, 2021

notmgsk Jan 27, 2021

erichulburd Jan 28, 2021

notmgsk Jan 27, 2021

stylewarning left a comment

erichulburd commented Apr 13, 2021

stylewarning commented Apr 15, 2021

notmgsk commented Oct 23, 2021

Make chip-contiguous-subschedule-from-last-instructions non-recursive #693

Make chip-contiguous-subschedule-from-last-instructions non-recursive #693

Conversation

erichulburd commented Jan 26, 2021 • edited by notmgsk Loading

erichulburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stylewarning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichulburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmgsk commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stylewarning left a comment

Choose a reason for hiding this comment

erichulburd commented Apr 13, 2021

stylewarning commented Apr 15, 2021

notmgsk commented Oct 23, 2021

erichulburd commented Jan 26, 2021 •

edited by notmgsk

Loading