qdk icon indicating copy to clipboard operation
qdk copied to clipboard

Provide a mechanism for defining simulator-only implementations of callables designed to be intrinsic for code generation

Open swernli opened this issue 1 year ago • 9 comments

Today, a user can provide an implementation (or decomposition) for an operation that they want to simulate and get that same implementation at code generation time (assuming it follows the restrictions for their chosen target) OR they can define the callable as body intrinsic and have it generated as an extern that must be supported by the chosen codegen target. If the user chooses the latter, they get their desired codegen behavior but cannot simulate their program, while the former works for simulation but may prevent QIR codegen or include undesired decomposition.

Given that, it has come up across several discussions that it would be convenient to have a mechanism whereby users could denote a callable as intrinsic for the purpose of codegen but still provide an implementation that gets used during simulation. What form this takes and how it is used in the language are up for discussion, though one possible approach could be a specialized attribute that denotes a callable that should be treated this way.

swernli avatar May 06 '24 20:05 swernli

The linked PR is one possible approach, but we can use this issue to discuss alternatives!

swernli avatar May 06 '24 20:05 swernli

I'm a bit concerned that just one attribute makes distinction kind-of implicit. When we have two implementations for two different profiles, we see that there're two implementations. Would it be more explicit if we have to implementations - one spelled out and one intrinsic, and decorate those for use in different cases?

DmitryVasilevsky avatar May 06 '24 21:05 DmitryVasilevsky

I can see this to be helpful beyond code generation. Maybe the attribute could be SimulatableIntrinsic and the implementation is only used by the simulator but intrinsic for all other evaluation backends.

msoeken avatar May 07 '24 06:05 msoeken

I'm a bit concerned that just one attribute makes distinction kind-of implicit. When we have two implementations for two different profiles, we see that there're two implementations. Would it be more explicit if we have to implementations - one spelled out and one intrinsic, and decorate those for use in different cases?

The reason I lean toward keeping them in one has to do with how we compile. If I have two definitions of a callable with the same name in the same scope, it produces a duplicate definition error. The way that works with @Config today is that the attribute forces one of those to be conditionally compiled out based on compilation target. We can't do that with a simulator because simulation is not a compilation target; you can be compiling for "Unrestricted" or "Base" and they both work with simulation. There might be ways to do some automatic implied target switching based on the scenario/feature we are compiling for, but I think there is other overhead to consider like whether both need to have doc comments and the behavior of things like go to definition... overall I prefer having a single item in this case.

swernli avatar May 07 '24 21:05 swernli

I can see this to be helpful beyond code generation. Maybe the attribute could be SimulatableIntrinsic and the implementation is only used by the simulator but intrinsic for all other evaluation backends.

I debated on the naming in the naming quite a bit in the linked PR... I used CodeGenIntrinsic but I think SimulatableIntrinsic could also work. The key thing I want to think of is how we would document the string and what the name would imply to someone who hasn't read the documentation. On the one hand, CodeGen describes when it's treated as intrinsic, but relies on knowing the shorthand for "code generation" and doesn't quite capture the fact that it's also intrinsic for circuits and resource estimation. Simulatable is also descriptive, but feels like an awkward term for some reason (that might just be in my head). I'm eager to hear what others think of the term.

swernli avatar May 07 '24 21:05 swernli

Another option that is possible but would require some more work in updating the parser is to have it not be attribute based at all but rather extend the existing body intrinsic generator to allow for a block. So we would support both the existing:

operation MyGate(q : Qubit) : Unit {
    body intrinsic;
}

and additionally support:

operation MyGate(q : Qubit) : Unit {
    body intrinsic {
        // Put simulator-only implementation here.
    }
}

While that might be a neat syntactic trick, I don't think it's as descriptive as having an attribute which at least provides a string for the user to search for in docs.

swernli avatar May 07 '24 21:05 swernli

A few more points that have come up in discussion over the last week:

  • Another benefit of attribute vs new "body" syntax is that it's easier to quickly turn it off and on, as you only need to add/remove or comment/uncomment one line.
  • @minestarks added a +1 to the suggestion from @msoeken to use a name that is more specific to simulation, since that's what the implementation is used for.

Given these two, I want to focus in on finding a good attribute name. Here's some we have brainstormed so far, would love to hear some more!

  1. SimulatableIntrinsic
  2. SimulationFallback
  3. SimulationOnly
  4. SimImpl

swernli avatar May 11 '24 18:05 swernli

Given these two, I want to focus in on finding a good attribute name. Here's some we have brainstormed so far, would love to hear some more!

  1. SimulatableIntrinsic
  2. SimulationFallback
  3. SimulationOnly
  4. SimImpl

I don't have more suggestions. I think all of them are very descriptive, but SimulationOnly could indicate that this operation cannot be used anywhere else except for simulation.

msoeken avatar May 13 '24 06:05 msoeken

  • Another benefit of attribute vs new "body" syntax is that it's easier to quickly turn it off and on, as you only need to add/remove or comment/uncomment one line.

@swernli I don't mind the attribute, but I will quibble with this... commenting out the @NameTBD() attribute has the same effect as commenting out body intrinsic in the hypothetical new syntax. Both are a one-line change.

I like SimulatorOnly.

FWIW I also like the idea of just reusing the @Config() attribute for this, and call it @Config(Simulator) . Yes we'd have to make a special case for it but I feel like we've taken enough liberties with how we implemented @Config() that I don't think this feels like a reach.

minestarks avatar May 13 '24 23:05 minestarks

Finally circling back to this... I think SimulatorOnly or SimulationOnly is nice and concise regarding what the implementation is used for, but I wonder if that doesn't convey enough about what the behavior will be when the simulator is not used, ie: during codegen, what happens to something marked as SimulationOnly?

To your first point regarding commenting out just body intrinsic, you are right, and it's making me reconsider that approach. Since I'm so concerned with how to convey both what happens during simulation and what happens during codegen, maybe that is reason enough to consider a new specialization, something like:

operation MyCustomOp(q : Qubit) : Unit {
    body intrinsic;
    simulation {
        // write stuff here...
    }
}

swernli avatar Jun 10 '24 19:06 swernli

Looking into the code, there is a wide gulf between what it takes to add an attribute like SimulationOnly or SimulatableIntrinsic vs adding a new specialization. Given that, I think we should only engage with a new specialization if there is a rather high value in usability/readability vs the attribute path.

swernli avatar Jun 10 '24 19:06 swernli