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

Nonrecursive contiguous subschedule #717

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erichulburd
Copy link
Contributor

@erichulburd erichulburd commented Jun 7, 2021

Rebases #693 which should fix tests.

Closes #697

@stylewarning
Copy link
Member

Both @stylewarning (me!) and @kilimanjaro will take a look.

Copy link
Contributor

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

It would help if we added another test with more than "H" gates (just using "H 0" and "H 1" does not allow us to tell whether the "H 0" gates get reordered accidentally), and perhaps some multi-qubit ops and classical instructions as well.

(let ((predecessors (gethash (pop stack) earlier-instrs)))
(dolist (predecessor predecessors)
(unless (gethash predecessor seen)
(setf (gethash predecessor seen) t)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is different from the earlier version -- previously, we had

(labels ((mark-predecessors-seen (instr)
               (cond ((resource= (instruction-resources instr)
                                 resource)
                      (setf (gethash instr seen) t))
                     ((resource-subsetp (instruction-resources instr)
                                        resource)
                      (map nil #'mark-predecessors-seen (gethash instr earlier-instrs)))))
...

which only marked those predecessors of instr which acted on a resource set equal to resource. Here you are marking everything -- is this the intent?

((and (resources-intersect-p resource (instruction-resources instr))
(resource-subsetp (instruction-resources instr) resource))
(push instr sched)
(dolist (earlier-instr (gethash instr earlier-instrs))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the ordering here matter?

It seems that previously there was an explicit reverse, but it wasn't entirely clear to me how the ordering of earlier-instructions was exploited (and e.g. I'm not sure whether the logical scheduler "officially" makes any promise about this ordering)? I realize that your code has gotten rid of the above reverse, but the use of a stack has the same effect (i.e. you push them one by one and then pop them in a reversed order), so if we are doing something which depends on the order I would ask that we leave a comment to that affect (an easy check for this is just to try the above iteration in reverse order).

@notmgsk
Copy link
Member

notmgsk commented Jul 1, 2021

I'm a bit sketched out by the original code, and also by the changes here.

Unnecessarily unfriendly language IMO

@braised-babbage
Copy link
Contributor

I'm a bit sketched out by the original code, and also by the changes here.

Unnecessarily unfriendly language IMO

I didn't mean it as a strong statement, but rather that it's hard for me to look at the code (e.g. even as originally written) and say if it is correct or not (or if it would break if the logical scheduler code changed, etc).

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.

Call stack exhaustion when compiling very deep programs
4 participants