loading the pulse calibrations #137

reza-j · 2023-07-26T17:28:40Z

This PR adds a pass that loads the pulse calibrations, mark the quir operations with their pulse calibration name, and add the pulse calibrations to the module

github-actions

clang-tidy made some suggestions

include/Conversion/QUIRToPulse/LoadPulseCals.h

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

clang-tidy made some suggestions

include/Conversion/QUIRToPulse/LoadPulseCals.h

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

github-actions · 2023-08-02T17:32:08Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-08-09T12:08:54Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions

clang-tidy made some suggestions

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

kitbarton

I think this is coming along really nicely.

There are a couple github checks that I've never seen before, but are good to fix.

I have a couple other general comments, which I flagged in a couple places but I think it would be good to change throughout this patch:

Error handling in the case that the pulse cals cannot be loaded (bad path, no file, etc).
Use of if (condition) assert(false && "message");. I think it is better to sink the condition into the assert. It improves readability (in my opinion) and makes things a bit easier to reason about in the case when asserts are disabled.

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

github-actions

clang-tidy made some suggestions

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

reza-j · 2023-09-11T18:12:59Z

@kitbarton gentle reminder that this PR is ready for a second review

mbhealy

A few changes would improve things. I'm also concerned by the OwningRef release semantics.

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

mbhealy · 2023-09-11T19:07:38Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  if (pulseCalsNameToSequenceMap.find(gateMangledName) !=
+      pulseCalsNameToSequenceMap.end()) {
+    // found a pulse calibration for the barrier gate
+    addPulseCalToModule(funcOp, pulseCalsNameToSequenceMap[gateMangledName]);
+  } else {


Same here, return early and drop the else.

addressed in
036e5b1

mbhealy · 2023-09-11T19:16:51Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  if (pulseCalsNameToSequenceMap.find(gateMangledName) !=
+      pulseCalsNameToSequenceMap.end()) {
+    // found a pulse calibration for the barrier gate
+    addPulseCalToModule(funcOp, pulseCalsNameToSequenceMap[gateMangledName]);
+  } else {


addressed in
036e5b1

mbhealy · 2023-09-11T19:17:02Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  if (pulseCalsNameToSequenceMap.find(gateMangledName) !=
+      pulseCalsNameToSequenceMap.end()) {
+    // found a pulse calibration for the gate
+    addPulseCalToModule(funcOp, pulseCalsNameToSequenceMap[gateMangledName]);
+  } else {


addressed in
036e5b1

mbhealy · 2023-09-11T19:20:58Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  if (pulseCalsAddedToIR.find(sequenceOp.sym_name().str()) ==
+      pulseCalsAddedToIR.end()) {
+    OpBuilder builder(funcOp.body());
+    auto *clonedPulseCalOp = builder.clone(*sequenceOp);
+    auto clonedPulseCalSequenceOp = dyn_cast<SequenceOp>(clonedPulseCalOp);
+    clonedPulseCalSequenceOp->moveBefore(funcOp);
+    pulseCalsAddedToIR.insert(sequenceOp.sym_name().str());
+  }


Would it be useful to add an else here with a debug message?

added in
80ddf0f

mbhealy · 2023-09-11T19:29:56Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  mlir::OwningOpRef<ModuleOp> pulseCalsModule(
+      mlir::parseSourceFile(sourceMgr, &getContext()));
+  if (!pulseCalsModule)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Failed to parse pulse calibrations file: " +
+                                       pulseCalsPath);
+  auto pulseCalsModuleRelease = pulseCalsModule.release();
+  pulseCalsModuleRelease->walk([&](mlir::pulse::SequenceOp sequenceOp) {
+    auto sequenceName = sequenceOp.sym_name().str();
+    pulseCalsNameToSequenceMap[sequenceName] = sequenceOp;
+  });


This whole section worries me. You parse in a module into an owning ref, then release it and store the name in a map. I'm worried this is only working currently because the memory is hanging around and not allocated for something else. Am I misunderstanding? It seems like there should be a copy of the contents of the parsed module into the existing top-level module for the translation unit.

This whole section worries me. You parse in a module into an owning ref, then release it and store the name in a map. I'm worried this is only working currently because the memory is hanging around and not allocated for something else. Am I misunderstanding? It seems like there should be a copy of the contents of the parsed module into the existing top-level module for the translation unit.

Doesn't this depend on whether the copy constructor for the sequenceOp performs a deep copy or shallow copy?
I couldn't find the definition of sequenceOp quickly, so I don't know for sure.
Since this is a map of string to sequenceOp (and not sequenceOp reference or pointer) I would assume each sequence op is being copied as part of adding it to the map.

so pulseCalsModule is of type mlir::OwningOpRef<ModuleOp>, and pulseCalsModule.release() returns the corresponding ModuleOp, and its result (i.e., pulseCalsModuleRelease) is of type ModuleOp which I then do further processing on. In other words, I used release function to obtain the ModuleOp. Please let me know what you think

Another example of this in the codebase is https://github.com/Qiskit/qss-compiler/blob/6528f4e7a7feeec7f72f784a4784151fb83b7a7c/lib/API/api.cpp#L611

@mbhealy @kitbarton was my answer related to your comment? or your concern is that maybe only a reference to sequenceOp is copied to the map, instead of the whole object?

From looking at the definition of owningOpRef it looks like the destructor only erases the memory if the op contains something, and release() should swap the memory from the owning ref to the moduleOp it's released to. Kit's point also about the copy for the map seems relevant. I'm still a bit worried because I'm not clear on exactly how Ops interact with the map constructs.

refactored and removed release() in 4dba7d8. Thanks for looking into this! @mbhealy

mbhealy · 2023-09-11T19:38:14Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+
+  // clone the body of the original sequence ops
+  for (std::size_t seqNum = 1; seqNum < sequenceOps.size(); seqNum++) {
+    builder.setInsertionPointAfter(&mergedSequenceOp.back().back());


Does insertion point setting have to be done inside the loop? Or can it be done once before the loop only?

addressed in
80ddf0f

mbhealy · 2023-09-11T19:45:00Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+    sequenceOp->setAttr("pulse.args", builder.getArrayAttr(argAttrVec));
+}
+
+bool LoadPulseCalsPass::areAllSequenceOpsHasSameDuration(


Slightly more grammatical function name

Suggested change

bool LoadPulseCalsPass::areAllSequenceOpsHasSameDuration(

bool LoadPulseCalsPass::doAllSequenceOpsHaveSameDuration(

addressed in
80ddf0f

kitbarton

I think this looks good.
I have a few non-blocking comments/questions.

@mbhealy had a good question about dangling memory for the sequenceOps that should get resolved before this is merged though.

kitbarton · 2023-09-13T19:57:22Z

include/Conversion/QUIRToPulse/LoadPulseCals.h

+
+  void addPulseCalToModule(FuncOp funcOp, mlir::pulse::SequenceOp sequenceOp);
+
+  // parse the pulse cals and add them to pulseCalsNameToSequenceMap


These are standard comments, not doxygen comments.
Is that intentional or an oversight?

this is just standard comment

kitbarton · 2023-09-13T20:11:26Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+    else if (auto castOp = dyn_cast<mlir::quir::DelayOp>(op))
+      loadPulseCals(castOp, callCircuitOp, funcOp);
+    else if (auto castOp = dyn_cast<mlir::quir::ResetQubitOp>(op))
+      loadPulseCals(castOp, callCircuitOp, funcOp);


Should there be a final else here if nothing matches? Or is that acceptable behaviour?

if nothing matches we need to make sure it's not a quantum gate/measure, that needs calibration loading. Added in
80ddf0f

added assert condition instead of assert false in 567e7fb

kitbarton · 2023-09-13T20:29:41Z

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

+  mlir::OwningOpRef<ModuleOp> pulseCalsModule(
+      mlir::parseSourceFile(sourceMgr, &getContext()));
+  if (!pulseCalsModule)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Failed to parse pulse calibrations file: " +
+                                       pulseCalsPath);
+  auto pulseCalsModuleRelease = pulseCalsModule.release();
+  pulseCalsModuleRelease->walk([&](mlir::pulse::SequenceOp sequenceOp) {
+    auto sequenceName = sequenceOp.sym_name().str();
+    pulseCalsNameToSequenceMap[sequenceName] = sequenceOp;
+  });


This whole section worries me. You parse in a module into an owning ref, then release it and store the name in a map. I'm worried this is only working currently because the memory is hanging around and not allocated for something else. Am I misunderstanding? It seems like there should be a copy of the contents of the parsed module into the existing top-level module for the translation unit.

Doesn't this depend on whether the copy constructor for the sequenceOp performs a deep copy or shallow copy?
I couldn't find the definition of sequenceOp quickly, so I don't know for sure.
Since this is a map of string to sequenceOp (and not sequenceOp reference or pointer) I would assume each sequence op is being copied as part of adding it to the map.

Co-authored-by: mbhealy <[email protected]>

github-actions

clang-tidy made some suggestions

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

mbhealy

LGTM!

github-actions · 2023-09-15T19:19:48Z

clang-tidy review says "All clean, LGTM! 👍"

first draft

12a516a

reza-j requested review from bcdonovan, taalexander, kitbarton and mbhealy July 26, 2023 17:29

github-actions bot reviewed Jul 26, 2023

View reviewed changes

reza-j and others added 3 commits August 1, 2023 16:04

Merge remote-tracking branch 'origin/main' into rj-load-pulse-cals

6ecd581

register the pass

0d5f050

Apply clang-tidy suggestions

d33dfdc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Aug 1, 2023

View reviewed changes

fix CI

fef8753

Merge remote-tracking branch 'origin/main' into rj-load-pulse-cals

383adf1

clean up and remove redundant delay args

b7ffd5e

github-actions bot reviewed Aug 15, 2023

View reviewed changes

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

kitbarton requested changes Aug 15, 2023

View reviewed changes

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Show resolved Hide resolved

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Show resolved Hide resolved

reza-j added 3 commits August 17, 2023 13:29

bug fixes and clean up

4d70ba7

addressing github comments

9c7e01e

parsing user specified pulse sequences

bf163de

github-actions bot reviewed Aug 17, 2023

View reviewed changes

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

addressing reviews

8f254c0

reza-j requested a review from kitbarton August 17, 2023 21:49

reza-j added 2 commits August 18, 2023 10:52

changing pulse.pulsecalname to pulse.calname

2f47a8c

fix for a minor bug

ad51a3a

mbhealy requested changes Sep 11, 2023

View reviewed changes

kitbarton approved these changes Sep 13, 2023

View reviewed changes

reza-j and others added 2 commits September 14, 2023 13:34

Update lib/Conversion/QUIRToPulse/LoadPulseCals.cpp

036e5b1

Co-authored-by: mbhealy <[email protected]>

addressing reviews

80ddf0f

Merge remote-tracking branch 'origin/main' into rj-load-pulse-cals

d57d61f

reza-j requested review from mbhealy and kitbarton September 14, 2023 19:30

reza-j added 3 commits September 15, 2023 09:15

fix the assert format

567e7fb

Merge remote-tracking branch 'origin/main' into rj-load-pulse-cals

0b7d4c1

refactor parsing pulse cals module

5619471

github-actions bot reviewed Sep 15, 2023

View reviewed changes

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

lib/Conversion/QUIRToPulse/LoadPulseCals.cpp Outdated Show resolved Hide resolved

reza-j and others added 2 commits September 15, 2023 11:56

Apply suggestions from code review

0222b01

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

refactor owning ref

4dba7d8

mbhealy approved these changes Sep 15, 2023

View reviewed changes

linux is not happy with std::greater

0d75cac

reza-j merged commit 12b9fdf into main Sep 15, 2023
2 checks passed

reza-j deleted the rj-load-pulse-cals branch September 15, 2023 20:00

	bool LoadPulseCalsPass::areAllSequenceOpsHasSameDuration(
	bool LoadPulseCalsPass::doAllSequenceOpsHaveSameDuration(


		void addPulseCalToModule(FuncOp funcOp, mlir::pulse::SequenceOp sequenceOp);

		// parse the pulse cals and add them to pulseCalsNameToSequenceMap

loading the pulse calibrations #137

loading the pulse calibrations #137

Conversation

reza-j commented Jul 26, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 2, 2023

github-actions bot commented Aug 9, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

kitbarton left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

reza-j commented Sep 11, 2023 • edited Loading

mbhealy 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitbarton 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

mbhealy left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 15, 2023

reza-j commented Sep 11, 2023 •

edited

Loading