search

quil-lang / quilc

The optimizing Quil compiler.
Apache License 2.0
454 stars 73 forks source link

Preserve existing name resolution when copying gate application #780

Closed braised-babbage closed 2 years ago

braised-babbage commented 2 years ago

Recently @karlosz observed that for some of the quilc tests, a large amount of time was spent calling Lisp's compile. This appears to still be the case, but I think I've tracked it down. For parametric programs (which are not currently tested in the benchmark-nq routines we have), quilc needs to construct a map from parameter values to matrices for each gate definition being used. This is part of gate-definition-gate here https://github.com/quil-lang/quilc/blob/master/src/gates.lisp#L386

Although a parsed program like RX(pi) 0; RX(pi) 1; RX(pi/2) 0 should end up with three applications, resolved to one gate definition (for "RX"), we were getting a new gate definition for every parametric gate application. The reason for this is that in the simplify-arithmetic pass, we were copying gate applications, and the implementation of copy-instance for these was deep on the name resolution.

This PR just changes that copy to be shallow for name resolution. I think this is a reasonable thing, even outside of this specific issue.

I'd be curious as to see what flame graph @karlosz is getting with this fix.

karlosz commented 2 years ago

We may also want to investigate removing runtime calls to compile all together, since closures sometimes can accomplish the same thing with very little overhead.

karlosz commented 2 years ago

This indeed seems to get rid of bad compile calls, from randomly sampling some flamegraphs. Before I would get things like:

Screen Shot 2021-11-24 at 11 59 26 Screen Shot 2021-11-24 at 12 00 43 Screen Shot 2021-11-24 at 12 00 16 Screen Shot 2021-11-24 at 11 59 55

Now I get graphs like:

Screen Shot 2021-11-24 at 14 24 44 Screen Shot 2021-11-24 at 14 18 14

![Uploading Screen Shot 2021-11-24 at 14.18.29.png…]()

Note that the profile for TEST-LOGICAL-MATRIX-SANITY looks completely different now. (And also appears to run much faster.)

stylewarning commented 2 years ago

We may also want to investigate removing runtime calls to compile all together, since closures sometimes can accomplish the same thing with very little overhead.

I think this will be harder than it seems.