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

Allow quantum registers returned from functions during buffer deallocation #1408

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dime10
Copy link
Contributor

@dime10 dime10 commented Jan 3, 2025

Context: This patch fixes a bug in upstream MLIR that prevents buffer deallocation from working in certain cases. The patch was also submitted upstream: llvm/llvm-project#121582

Description of the Change: This PR adds a patch to the llvm-project source, and updates the relevant build recipes. The llvm step during wheel builds is now also handled through Make instead of replicating the CMake configuration in each wheel script.

Benefits: Unblocks our friends over at Qrisp.

Possible Drawbacks: Patching dependencies is not great, but hopefully temporary.

Related GitHub Issues:
[sc-81444]

@dime10 dime10 added bug Something isn't working upstream Issue with an upstream project labels Jan 3, 2025
@dime10 dime10 requested a review from erick-xanadu January 3, 2025 23:23
Copy link
Contributor

github-actions bot commented Jan 3, 2025

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@dime10 dime10 added the author:build-wheels Run the wheel building workflows on this Pull Request label Jan 3, 2025
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Thanks for using the make recipes. In the future we can add the patches to the cache's hash. It looks like the build process failed, I'll give you some more time to work on it. Let me know again once it is succeeding.

@@ -75,10 +82,12 @@ llvm:
-DCMAKE_CXX_VISIBILITY_PRESET=$(SYMBOL_VISIBILITY)

# TODO: when updating LLVM, test to see if mlir/unittests/Bytecode/BytecodeTest.cpp:55 is passing
# and remove filter
LIT_FILTER_OUT="Bytecode" cmake --build $(LLVM_BUILD_DIR) --target check-mlir llvm-symbolizer
# and remove filter. This tests fails on CI/CD not locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# and remove filter. This tests fails on CI/CD not locally.
# and remove filter. These tests fails on CI/CD not locally.

mlir/Makefile Show resolved Hide resolved
@dime10 dime10 force-pushed the buffer-deallocation-patch branch from 2a772be to f9f4e84 Compare January 6, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request bug Something isn't working upstream Issue with an upstream project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants