ensure applicable symbolic-accepting compilers are included #669
Conversation
When we changed the way occ-tbl metrics were calculated, it caused
quilc to find particularly good collections of compilers ("compiler
paths") for certain gate sets. These particularly good compiler paths
were better than before! But there was a hitch... the better compilers
were also less flexible in what they matched against, though that
wasn't apparent from the binding patterns themselves. In particular,
in some cases, the better compilers were ones that also did _not_
accept symbolic parameters (e.g., a memory reference). This means
these better compilers failed to take into account inputs like
`RX(theta) 0` even though the pattern `RX(_) _` was compilable.
The fix in this commit has two facets:
- First, we use heuristics to determine if a compiler can accept
symbolic parameters. This heuristic can be improved, but works
fine for one-parameter compilers, which are the real-world cases
that failed.
- Second, even if we find a better compiler path when warming the
chip spec, as long as a symbolic-accepting path was found, it's
included in the list of compilers. This may still fail, if the
entire path doesn't allow symbolic compilers. This was too
complicated to check for this commit with too little gain.
This fixes both issues reported in issue quil-lang#667.
stylewarning
force-pushed
the
sturdier-compilers
branch
from
87b2099 to
6dd33df
Compare
There was a problem hiding this comment.
I definitely approve of the goals of this PR. I think there are three classes of "wildcards" to be treated:
- Truly permissive wildcards.
- Wildcards that allow certain arithmetic expressions in memory references.
- Wildcards that don't allow memory references at all.
Imperfectly separating these into two buckets is already an improvement.
My only real concern is that we test how this PR interacts with PAULI-SUM applications with several memory parameters (e.g., the P-S definition of the canonical gate). Add a test for that and I have no objections at all.
| (defun symbolic-param-generator () | ||
| (let ((n -1)) | ||
| (lambda () | ||
| (make-delayed-expression nil nil `(* 1.0 ,(mref "G" (incf n))))))) |
There was a problem hiding this comment.
Gross expressions like this one make me feel the AST stuff around delayed expressions should be seriously rethought. There's nothing to do about it here, but someone (perhaps me) should open an issue about it.
| @@ -405,6 +425,15 @@ What \"concrete\" means depends on the types of A and B: | |||
| :initarg :output-gates | |||
| :reader compiler-output-gates | |||
| :documentation "Information automatically extracted about the target gate set.") | |||
| (info-plist | |||
There was a problem hiding this comment.
Beware: misc-data was not a great idea in chip-specification, and the short-form docs for this sound OK (cached info! sure!) the details are sketchier (in what sense is :SYMBOLIC caching anything?). Are we sure this tag doesn't belong at top-level in this object?
| "Does COMPILER accept instructions with symbolic parameters? | ||
|
|
||
| Compilers matching gates which don't have parameters don't count as accepting parameters." | ||
| (let ((info (getf (compiler-info-plist compiler) ':SYMBOLIC '#1=#:unknown))) |
There was a problem hiding this comment.
IMO getf should return a secondary value like gethash does. Where do I file a complaint?
| ;; - At least one binding takes an arbitrary parameter. | ||
| (let ((*instantiate-binding-with-symbolic-parameters* t)) | ||
| (let ((prototype-instructions (map 'list #'instantiate-binding bindings))) | ||
| ;; We are ready to lock & load. |
| (let ((parameter-lists (map 'list #'gate-binding-parameters bindings))) | ||
| ;; HEURISTIC ALERT: All bindings have fewer than two | ||
| ;; parameters. This rule doesn't necessarily mean a compiler | ||
| ;; doesn't accept symbolic parameters, but it is unlikely in |
There was a problem hiding this comment.
Is this also unlikely when using parametric Pauli sums?
| (when (or (compiler-allows-symbolic-parameters-p compiler) | ||
| (and (not (path-has-a-loop-p special-path (compiler-bindings compiler))) | ||
| (>= special-cost generic-cost))) | ||
| ;; then store it ya dingus! |
There was a problem hiding this comment.
I agree on the pauli sum point @ecpeterson brought up. Otherwise, this is a great start for symbolic parameter infrastructure!
| @@ -277,3 +277,42 @@ CNOT 4 8 | |||
|
|
|||
| (deftest test-free-rx-rz-strings-reduce () | |||
| (%test-reduction-with-chip 3 (%read-test-chipspec "1q-free-rx.qpu"))) | |||
|
|
|||
| (deftest test-symbolic-parameter-compiles-in-examples () | |||
There was a problem hiding this comment.
Is it possible to split this up into tests for the new symbolic parameter detection and then another for compiler hooks directly from #667?
|
I remembered this morning that the "symbolic" compilers for PAULI-SUM applications permit several parameters but only one memory reference. How close is that to being covered with what's already here? |
|
I think it’s a TODO to file issues with ECP’s comments. |
When we changed the way occ-tbl metrics were calculated, it caused
quilc to find particularly good collections of compilers ("compiler
paths") for certain gate sets. These particularly good compiler paths
were better than before! But there was a hitch... the better compilers
were also less flexible in what they matched against, though that
wasn't apparent from the binding patterns themselves. In particular,
in some cases, the better compilers were ones that also did not
accept symbolic parameters (e.g., a memory reference). This means
these better compilers failed to take into account inputs like
RX(theta) 0even though the patternRX(_) _was compilable.The fix in this commit has two facets:
First, we use heuristics to determine if a compiler can accept
symbolic parameters. This heuristic can be improved, but works
fine for one-parameter compilers, which are the real-world cases
that failed.
Second, even if we find a better compiler path when warming the
chip spec, as long as a symbolic-accepting path was found, it's
included in the list of compilers. This may still fail, if the
entire path doesn't allow symbolic compilers. This was too
complicated to check for this commit with too little gain.
This fixes both issues reported in issue #667.